Re: [XEN PATCH v10 2/5] x86/pvh: Allow (un)map_pirq when dom0 is PVH

["Chen, Jiqian" <Jiqian.Chen@xxxxxxx>]

On 2024/6/18 16:38, Jan Beulich wrote:
> On 18.06.2024 08:49, Chen, Jiqian wrote:
>> On 2024/6/17 22:45, Jan Beulich wrote:
>>> On 17.06.2024 11:00, Jiqian Chen wrote:
>>>> If run Xen with PVH dom0 and hvm domU, hvm will map a pirq for
>>>> a passthrough device by using gsi, see qemu code
>>>> xen_pt_realize->xc_physdev_map_pirq and libxl code
>>>> pci_add_dm_done->xc_physdev_map_pirq. Then xc_physdev_map_pirq
>>>> will call into Xen, but in hvm_physdev_op, PHYSDEVOP_map_pirq
>>>> is not allowed because currd is PVH dom0 and PVH has no
>>>> X86_EMU_USE_PIRQ flag, it will fail at has_pirq check.
>>>>
>>>> So, allow PHYSDEVOP_map_pirq when dom0 is PVH and also allow
>>>> PHYSDEVOP_unmap_pirq for the failed path to unmap pirq.
>>>
>>> Why "failed path"? Isn't unmapping also part of normal device removal
>>> from a guest?
>> Yes, both. I will change to also "allow PHYSDEVOP_unmap_pirq for the device 
>> removal path to unmap pirq".
>>
>>>
>>>> And
>>>> add a new check to prevent self map when subject domain has no
>>>> PIRQ flag.
>>>
>>> You still talk of only self mapping, and the code also still does only
>>> that. As pointed out before: Why would you allow mapping into a PVH
>>> DomU? IOW what purpose do the "d == currd" checks have?
>> The checking I added has two purpose, first is I need to allow this case:
>> Dom0(without PIRQ) + DomU(with PIRQ), because the original code just do 
>> (!has_pirq(currd)) will cause map_pirq fail in this case.
>> Second I need to disallow self-mapping:
>> DomU(without PIRQ) do map_pirq, the "d==currd" means the currd is the 
>> subject domain itself.
>>
>> Emmm, I think I know what's your concern.
>> Do you mean I need to
>> " Prevent map_pirq when currd has no X86_EMU_USE_PIRQ flag "
>> instead of
>> " Prevent self-map when currd has no X86_EMU_USE_PIRQ flag ",
> 
> No. What I mean is that I continue to fail to see why you mention "currd".
> IOW it would be more like "prevent mapping when the subject domain has no
> X86_EMU_USE_PIRQ" (which, as a specific sub-case, includes self-mapping
> if the caller specifies DOMID_SELF for the subject domain).
Oh, I see, not only to prevent self-mapping, but if the subject domain has no 
PIRQs, we should reject, self-mapping is just the one sub case.

> 
>> so I need to remove "d==currd", right?
> 
> Removing this check is what I'm after, yes. Yet that's not in sync with
> either of the two quoted sentences above.
> 
>>>> So that domU with PIRQ flag can success to map pirq for
>>>> passthrough devices even dom0 has no PIRQ flag.
>>>
>>> There's still a description problem here. Much like the first sentence,
>>> this last one also says that the guest would itself map the pIRQ. In
>>> which case there would still not be any reason to expose the sub-
>>> functions to Dom0.
>> If change to " So that the interrupt of a passthrough device can success to 
>> be mapped to pirq for domU with PIRQ flag when dom0 is PVH.",
>> Is it OK?
> 
> Kind of, yes. "can be successfully mapped" is one of the various possibilities
> of making this read a little more smoothly.
OK.

> 
> Jan

-- 
Best regards,
Jiqian Chen.