Re: [PATCH v10 1/6] VT-d: Introduce new fields in msi_desc to track binding with guest interrupt

[Chao Gao <chao.gao@intel.com>]

On Wed, Mar 15, 2017 at 10:41:13AM -0600, Jan Beulich wrote:
>>>> On 15.03.17 at 06:11, <chao.gao@intel.com> wrote:
>> --- a/xen/drivers/passthrough/vtd/intremap.c
>> +++ b/xen/drivers/passthrough/vtd/intremap.c
>> @@ -552,11 +552,12 @@ static int msi_msg_to_remap_entry(
>>      struct msi_desc *msi_desc, struct msi_msg *msg)
>>  {
>>      struct iremap_entry *iremap_entry = NULL, *iremap_entries;
>> -    struct iremap_entry new_ire;
>> +    struct iremap_entry new_ire = {{0}};
>
>Any reason this isn't simple "{ }"?
>

I also think '{}' can work. But here is the compiler output:
intremap.c: In function ‘msi_msg_to_remap_entry’:
intremap.c:587:12: error: missing braces around initializer [-Werror=missing-braces]
     struct iremap_entry new_ire = {0};
            ^
intremap.c:587:12: error: (near initialization for ‘new_ire.<anonymous>’) [-Werror=missing-braces]

>> @@ -595,33 +596,35 @@ static int msi_msg_to_remap_entry(
>>      GET_IREMAP_ENTRY(ir_ctrl->iremap_maddr, index,
>>                       iremap_entries, iremap_entry);
>>  
>> -    memcpy(&new_ire, iremap_entry, sizeof(struct iremap_entry));
>> -
>> -    /* Set interrupt remapping table entry */
>> -    new_ire.remap.fpd = 0;
>> -    new_ire.remap.dm = (msg->address_lo >> MSI_ADDR_DESTMODE_SHIFT) & 0x1;
>> -    new_ire.remap.tm = (msg->data >> MSI_DATA_TRIGGER_SHIFT) & 0x1;
>> -    new_ire.remap.dlm = (msg->data >> MSI_DATA_DELIVERY_MODE_SHIFT) & 0x1;
>> -    /* Hardware require RH = 1 for LPR delivery mode */
>> -    new_ire.remap.rh = (new_ire.remap.dlm == dest_LowestPrio);
>> -    new_ire.remap.avail = 0;
>> -    new_ire.remap.res_1 = 0;
>> -    new_ire.remap.vector = (msg->data >> MSI_DATA_VECTOR_SHIFT) &
>> -                            MSI_DATA_VECTOR_MASK;
>> -    new_ire.remap.res_2 = 0;
>> -    if ( x2apic_enabled )
>> -        new_ire.remap.dst = msg->dest32;
>> +    if ( !pi_desc )
>> +    {
>> +        new_ire.remap.dm = msg->address_lo >> MSI_ADDR_DESTMODE_SHIFT;
>> +        new_ire.remap.tm = msg->data >> MSI_DATA_TRIGGER_SHIFT;
>> +        new_ire.remap.dlm = msg->data >> MSI_DATA_DELIVERY_MODE_SHIFT;
>> +        /* Hardware require RH = 1 for LPR delivery mode */
>
>As you're touching this anyway, please make it "requires" and
>"lowest priority" respectively.

OK.

>
>> @@ -968,59 +927,14 @@ int pi_update_irte(const struct vcpu *v, const struct pirq *pirq,
>>          rc = -ENODEV;
>>          goto unlock_out;
>>      }
>> -
>> -    pci_dev = msi_desc->dev;
>> -    if ( !pci_dev )
>> -    {
>> -        rc = -ENODEV;
>> -        goto unlock_out;
>> -    }
>> -
>> -    remap_index = msi_desc->remap_index;
>> +    msi_desc->pi_desc = pi_desc;
>> +    msi_desc->gvec = gvec;
>
>Am I overlooking something - I can't seem to find any place where these
>two fields (or at least the former) get cleared again? This may be correct,
>but if it is the reason wants recording in the commit message.

IIUC, the current logic is free the whole msi_desc when device deassignment.
But it is better to clear this two fields explicitly. I will call pi_update_irte()
with @vcpu=NULL, just like Patch [6/6] when device deassignment.
Do you think the new added code should be squashed into this one?
Shall I also squash Patch [6/6] to this one? I think it is to fix another flaw.

>
>> --- a/xen/include/asm-x86/msi.h
>> +++ b/xen/include/asm-x86/msi.h
>> @@ -118,6 +118,8 @@ struct msi_desc {
>>  	struct msi_msg msg;		/* Last set MSI message */
>>  
>>  	int remap_index;		/* index in interrupt remapping table */
>> +	const void *pi_desc;		/* PDA, indicates msi is delivered via VT-d PI */
>
>Why "void"? Please let's play type safe wherever we can.

Ok.

Thank
Chao
>
>Jan
>

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