Re: [Xen-devel] [PATCH v3 5/6] xen/x86: add PHYSDEVOP_msi_msix_set_enable

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 30.01.19 at 14:51, <roger.pau@xxxxxxxxxx> wrote:
> On Sat, Jan 26, 2019 at 03:31:16AM +0100, Marek Marczykowski-Górecki wrote:
>> --- a/xen/arch/x86/msi.c
>> +++ b/xen/arch/x86/msi.c
>> @@ -1474,6 +1474,22 @@ int pci_restore_msi_state(struct pci_dev *pdev)
>>      return 0;
>>  }
>>  
>> +int msi_msix_set_enable(struct pci_dev *pdev, int flag, int enable)
>> +{
>> +    if ( !current->domain->target || pdev->domain != 
>> current->domain->target )
>> +        return -EPERM;
>> +
>> +    switch ( flag ) {
>> +        case PHYSDEVOP_MSI_MSIX_SET_ENABLE_MSI:
>> +            msi_set_enable(pdev, enable);
>> +            break;
> 
> Please add a newline here.

And also please correct indentation. The misplaced { was
already pointed out iirc.

>> +        case PHYSDEVOP_MSI_MSIX_SET_ENABLE_MSIX:
>> +            msix_set_enable(pdev, enable);
>> +            break;
>> +    }
>> +    return 0;

There's another blank line missing above here.

>> --- a/xen/include/public/physdev.h
>> +++ b/xen/include/public/physdev.h
>> @@ -344,6 +344,19 @@ struct physdev_dbgp_op {
>>  typedef struct physdev_dbgp_op physdev_dbgp_op_t;
>>  DEFINE_XEN_GUEST_HANDLE(physdev_dbgp_op_t);
>>  
>> +#define PHYSDEVOP_MSI_MSIX_SET_ENABLE_MSI  0
>> +#define PHYSDEVOP_MSI_MSIX_SET_ENABLE_MSIX 1
>> +
>> +#define PHYSDEVOP_msi_msix_set_enable   32
> 
> There's no need for the 'msi_msix' name, there are already other
> hypercalls that deal with both msi and msix and just have msi in the
> name: PHYSDEVOP_msi_set_enable.

And FAOD the same then also for the other two defines,
or alternatively

#define PHYSDEVOP_MSI_SET_ENABLE  0
#define PHYSDEVOP_MSIX_SET_ENABLE 1

>> +struct physdev_msi_msix_set_enable {
>> +    /* IN */
>> +    struct physdev_pci_device pci;
>> +    uint8_t flag;
> 
> But this is not really a flags field, I would rather rename this
> to 'mode' maybe.
> 
>> +    uint8_t enable;
>> +};
>> +typedef struct physdev_msi_msix_set_enable physdev_msi_msix_set_enable_t;
>> +DEFINE_XEN_GUEST_HANDLE(physdev_msi_msix_set_enable_t);
> 
> I think you need to add the new hypercall to include/xlat.lst, AFAICT
> it requires no translation, so you should add it as '?'.

Plus add invocation of the resulting macro.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel