Re: [PATCH 1/2] Xen: enabling emulated MSI injection

[Wei Liu <liuw@liuw.name>]

On Thu, May 26, 2011 at 1:26 PM, Tian, Kevin <kevin.tian@intel.com> wrote:
>> From: Wei Liu [mailto:liuw@liuw.name]
>> Sent: Thursday, May 26, 2011 11:30 AM
>>
>> On Thu, May 26, 2011 at 11:21 AM, Tian, Kevin <kevin.tian@intel.com> wrote:
>> >> From: Wei Liu
>> >> Sent: Thursday, May 26, 2011 11:09 AM
>> >>
>> >> commit 176dc2a26b4b9dd0fe30fab3b168722766218245
>> >> Author: Wei Liu <liuw@liuw.name>
>> >> Date:   Thu May 26 10:23:01 2011 +0800
>> >>
>> >>     x86: Add a new operation in HVMOP to inject emulated MSI.
>> >>
>> >>     The original vmsi_deliver is renamed to vmsi_deliver_irq. New
>> >
>> > in your patch you mean vmsi_deliver_pirq which is inconsistent.
>
> the comment says vmsi_deliver_irq while your code has vmsi_deliver_pirq...
>
>> >
>> >>     vmsi_deliver is dedicated to the actually delivering.
>> >>
>> >>     Signed-off-by: Wei Liu <liuw@liuw.name>
>> >>
>> >> --- a/xen/include/public/hvm/hvm_op.h
>> >> +++ b/xen/include/public/hvm/hvm_op.h
>> >> @@ -82,11 +82,24 @@ typedef enum {
>> >>      HVMMEM_mmio_dm,            /* Reads and write go to the
>> device
>> >> model */
>> >>  } hvmmem_type_t;
>> >>
>> >> +/* MSI injection for emulated devices */
>> >> +#define HVMOP_inj_msi         6
>> >> +struct xen_hvm_inj_msi {
>> >> +    /* Domain to be injected */
>> >> +    domid_t   domid;
>> >> +    /* Address (0xfeeXXXXX) */
>> >> +    uint64_t  addr;
>> >> +    /* Data -- lower 32 bits */
>> >> +    uint32_t  data;
>> >> +};
>> >> +typedef struct xen_hvm_inj_msi xen_hvm_inj_msi_t;
>> >> +DEFINE_XEN_GUEST_HANDLE(xen_hvm_inj_msi_t);
>> >> +
>> >>  /* Following tools-only interfaces may change in future. */
>> >>  #if defined(__XEN__) || defined(__XEN_TOOLS__)
>> >>
>> >>  /* Track dirty VRAM. */
>> >> -#define HVMOP_track_dirty_vram    6
>> >> +#define HVMOP_track_dirty_vram    7
>> >>  struct xen_hvm_track_dirty_vram {
>> >>      /* Domain to be tracked. */
>> >>      domid_t  domid;
>> >
>> > This breaks backward API compatibility.
>> >
>> > Thanks
>> > Kevin
>> >
>>
>> Stefano suggests this rename.
>>
>> The only function that invokes vmsi_deliver() is hvm_pci_msi_assert(),
>> IIRC. That has been taken care of.
>>
>
> You didn't catch my comment.
>
> you change existing HVMOP command index which breaks backward
> compatibility.
>
> Thanks
> Kevin
>

Hmm... I see.

Keir pointed out that problem too. I will revise it and push this patch again.

-- 
Best regards
Wei Liu
Twitter: @iliuw
Site: http://liuw.name