Re: [Xen-devel] [PATCH v3 10/10] x86/MSI-X: provide hypercall interface for mask-all control

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 05.06.15 at 17:57, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 05/06/15 12:28, Jan Beulich wrote:
>> Qemu shouldn't be fiddling with this bit directly, as the hypervisor
>> may (and now does) use it for its own purposes. Provide it with a
>> replacement interface, allowing the hypervisor to track host and guest
>> masking intentions independently (clearing the bit only when both want
>> it clear).
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> ---
>> Whether the permission check should really be an XSM_TARGET one needs
>> to be determined: That allowing the guest to issue the hypercalls on
>> itself means permitting it to bypass the device model, and thus render
>> device model state stale.
> 
> I concur.
> 
> On the other hand, there should be nothing the guest could do with
> access to these hypercalls which would damage Xen itself.

That's exactly the reason I have it as it is now.

>> --- a/tools/libxc/xc_physdev.c
>> +++ b/tools/libxc/xc_physdev.c
>> @@ -112,3 +112,38 @@ int xc_physdev_unmap_pirq(xc_interface *
>>      return rc;
>>  }
>>  
>> +int xc_physdev_msix_enable(xc_interface *xch,
>> +                           int segment,
>> +                           int bus,
>> +                           int devfn,
>> +                           int on)
>> +{
>> +    struct physdev_pci_device dev = {
>> +        .seg = segment,
>> +        .bus = bus,
>> +        .devfn = devfn
>> +    };
>> +
>> +    return do_physdev_op(xch,
>> +                         on ? PHYSDEVOP_msix_enable
>> +                            : PHYSDEVOP_msix_disable,
>> +                         &dev, sizeof(dev));
>> +}
> 
> xc_physdev_misx_enable(xch, X, Y, Z, 0) is slightly misleading to read...
> 
>> +
>> +int xc_physdev_msix_mask_all(xc_interface *xch,
>> +                             int segment,
>> +                             int bus,
>> +                             int devfn,
>> +                             int mask)
>> +{
>> +    struct physdev_pci_device dev = {
>> +        .seg = segment,
>> +        .bus = bus,
>> +        .devfn = devfn
>> +    };
>> +
>> +    return do_physdev_op(xch,
>> +                         mask ? PHYSDEVOP_msix_mask_all
>> +                              : PHYSDEVOP_msix_unmask_all,
>> +                         &dev, sizeof(dev));
>> +}
> 
> And equally xc_physdev_misx_mask_all(xch, X, Y, Z, 0).
> 
> I think it would be cleaner to have something like
> 
> xc_physdev_msix_manage(xch, X, Y, Z, PHYSDEVOP_msix_XXX)
> 
> which results in far more readable client code.

But will require callers to include xen/physdev.h which I think
otherwise they wouldn't have to. Anyway, I'll wait for what the
tool stack and qemu maintainers say.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel