| Message ID | 20230312075455.450187-5-ray.huang@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce VirtIO GPU and Passthrough GPU support on Xen PVH dom0 | expand |
On 12.03.2023 08:54, Huang Rui wrote:
> From: Chen Jiqian <Jiqian.Chen@amd.com>
An empty description won't do here. First of all you need to address the Why?
As already hinted at in the reply to the earlier patch, it looks like you're
breaking the intended IRQ model for PVH.
Jan
On 14/03/2023 4:30 pm, Jan Beulich wrote: > On 12.03.2023 08:54, Huang Rui wrote: >> From: Chen Jiqian <Jiqian.Chen@amd.com> > An empty description won't do here. First of all you need to address the Why? > As already hinted at in the reply to the earlier patch, it looks like you're > breaking the intended IRQ model for PVH. I think this is rather unfair. Until you can point to the document which describes how IRQs are intended to work in PVH, I'd say this series is pretty damn good attempt to make something that functions, in the absence of any guidance. ~Andrew P.S. If it isn't obvious, this is a giant hint that something should be written down...
On Wed, 15 Mar 2023, Andrew Cooper wrote: > On 14/03/2023 4:30 pm, Jan Beulich wrote: > > On 12.03.2023 08:54, Huang Rui wrote: > >> From: Chen Jiqian <Jiqian.Chen@amd.com> > > An empty description won't do here. First of all you need to address the Why? > > As already hinted at in the reply to the earlier patch, it looks like you're > > breaking the intended IRQ model for PVH. > > I think this is rather unfair. > > Until you can point to the document which describes how IRQs are > intended to work in PVH, I'd say this series is pretty damn good attempt > to make something that functions, in the absence of any guidance. And to make things more confusing those calls are not needed for PVH itself, those calls are needed so that we can run QEMU to support regular HVM guests on PVH Dom0 (I'll let Ray confirm.) So technically, this is not breaking the PVH IRQ model.
On Wed, 15 Mar 2023, Stefano Stabellini wrote: > On Wed, 15 Mar 2023, Andrew Cooper wrote: > > On 14/03/2023 4:30 pm, Jan Beulich wrote: > > > On 12.03.2023 08:54, Huang Rui wrote: > > >> From: Chen Jiqian <Jiqian.Chen@amd.com> > > > An empty description won't do here. First of all you need to address the Why? > > > As already hinted at in the reply to the earlier patch, it looks like you're > > > breaking the intended IRQ model for PVH. > > > > I think this is rather unfair. > > > > Until you can point to the document which describes how IRQs are > > intended to work in PVH, I'd say this series is pretty damn good attempt > > to make something that functions, in the absence of any guidance. > > And to make things more confusing those calls are not needed for PVH > itself, those calls are needed so that we can run QEMU to support > regular HVM guests on PVH Dom0 (I'll let Ray confirm.) > > So technically, this is not breaking the PVH IRQ model. To add more info: QEMU (hw/xen/xen_pt.c) calls xc_physdev_map_pirq and xc_domain_bind_pt_pci_irq. Note that xc_domain_bind_pt_pci_irq is the key hypercall here and it takes a pirq as parameter. That is why QEMU calls xc_physdev_map_pirq, so that we can get the pirq and use the pirq as parameter for xc_domain_bind_pt_pci_irq. We need to get the above to work also with Dom0 PVH.
On 15.03.2023 18:01, Andrew Cooper wrote: > On 14/03/2023 4:30 pm, Jan Beulich wrote: >> On 12.03.2023 08:54, Huang Rui wrote: >>> From: Chen Jiqian <Jiqian.Chen@amd.com> >> An empty description won't do here. First of all you need to address the Why? >> As already hinted at in the reply to the earlier patch, it looks like you're >> breaking the intended IRQ model for PVH. > > I think this is rather unfair. > > Until you can point to the document which describes how IRQs are > intended to work in PVH, I'd say this series is pretty damn good attempt > to make something that functions, in the absence of any guidance. Are you advocating for patches which don't explain why they make a certain change? Even in the absence of any documentation, the code itself can be taken as reference, and hence it can be pointed out that either something was wrong before, or something needs extending in a certain way to make some use case work which can't be mode work by other means. In the case of this series, without knowing the "Why?" for the various changes, it is also impossible to suggest alternative approaches. Jan
On 16.03.2023 01:26, Stefano Stabellini wrote: > On Wed, 15 Mar 2023, Andrew Cooper wrote: >> On 14/03/2023 4:30 pm, Jan Beulich wrote: >>> On 12.03.2023 08:54, Huang Rui wrote: >>>> From: Chen Jiqian <Jiqian.Chen@amd.com> >>> An empty description won't do here. First of all you need to address the Why? >>> As already hinted at in the reply to the earlier patch, it looks like you're >>> breaking the intended IRQ model for PVH. >> >> I think this is rather unfair. >> >> Until you can point to the document which describes how IRQs are >> intended to work in PVH, I'd say this series is pretty damn good attempt >> to make something that functions, in the absence of any guidance. > > And to make things more confusing those calls are not needed for PVH > itself, those calls are needed so that we can run QEMU to support > regular HVM guests on PVH Dom0 (I'll let Ray confirm.) Ah, but that wasn't said anywhere, was it? In which case ... > So technically, this is not breaking the PVH IRQ model. ... I of course agree here. But then I guess we may want to reject attempts for a domain to do any of this to itself. Jan
On Thu, Mar 16, 2023 at 09:51:20AM +0100, Jan Beulich wrote: > On 16.03.2023 01:26, Stefano Stabellini wrote: > > On Wed, 15 Mar 2023, Andrew Cooper wrote: > >> On 14/03/2023 4:30 pm, Jan Beulich wrote: > >>> On 12.03.2023 08:54, Huang Rui wrote: > >>>> From: Chen Jiqian <Jiqian.Chen@amd.com> > >>> An empty description won't do here. First of all you need to address the Why? > >>> As already hinted at in the reply to the earlier patch, it looks like you're > >>> breaking the intended IRQ model for PVH. > >> > >> I think this is rather unfair. > >> > >> Until you can point to the document which describes how IRQs are > >> intended to work in PVH, I'd say this series is pretty damn good attempt > >> to make something that functions, in the absence of any guidance. > > > > And to make things more confusing those calls are not needed for PVH > > itself, those calls are needed so that we can run QEMU to support > > regular HVM guests on PVH Dom0 (I'll let Ray confirm.) > > Ah, but that wasn't said anywhere, was it? In which case ... > > > So technically, this is not breaking the PVH IRQ model. > > ... I of course agree here. But then I guess we may want to reject > attempts for a domain to do any of this to itself. For PCI passthrough we strictly need the PHYSDEVOP_{un,}map_pirq because that's the only way QEMU currently has to allocate MSI(-X) vectors from physical devices in order to assign to guests. We could see about moving those to DM ops maybe in the future, as I think it would be clearer, but that shouldn't block the work here. If we start allowing PVH domains to use PIRQs we must enforce that PIRQ cannot be mapped to event channels, IOW, block EVTCHNOP_bind_pirq. Thanks, Roger.
On Wed, Mar 15, 2023 at 12:30:21AM +0800, Jan Beulich wrote: > On 12.03.2023 08:54, Huang Rui wrote: > > From: Chen Jiqian <Jiqian.Chen@amd.com> > > An empty description won't do here. First of all you need to address the Why? > As already hinted at in the reply to the earlier patch, it looks like you're > breaking the intended IRQ model for PVH. > Sorry, I used a wrong patch without commit message. Will fix in next version. Thanks, Ray
On Thu, Mar 16, 2023 at 01:01:52AM +0800, Andrew Cooper wrote: > On 14/03/2023 4:30 pm, Jan Beulich wrote: > > On 12.03.2023 08:54, Huang Rui wrote: > >> From: Chen Jiqian <Jiqian.Chen@amd.com> > > An empty description won't do here. First of all you need to address the Why? > > As already hinted at in the reply to the earlier patch, it looks like you're > > breaking the intended IRQ model for PVH. > > I think this is rather unfair. > > Until you can point to the document which describes how IRQs are > intended to work in PVH, I'd say this series is pretty damn good attempt > to make something that functions, in the absence of any guidance. > Thank you, Andrew! This is the first time we submit Xen patches, any comments are warm for us. :-) Thanks, Ray
diff --git a/xen/arch/x86/hvm/hypercall.c b/xen/arch/x86/hvm/hypercall.c index 16a2f5c0b3..fce786618c 100644 --- a/xen/arch/x86/hvm/hypercall.c +++ b/xen/arch/x86/hvm/hypercall.c @@ -89,6 +89,7 @@ long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) case PHYSDEVOP_eoi: case PHYSDEVOP_irq_status_query: case PHYSDEVOP_get_free_pirq: + case PHYSDEVOP_setup_gsi: break; case PHYSDEVOP_pci_mmcfg_reserved: