[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v10 1/6] VT-d: Introduce new fields in msi_desc to track binding with guest interrupt
On Wed, Mar 15, 2017 at 10:41:13AM -0600, Jan Beulich wrote: >>>> On 15.03.17 at 06:11, <chao.gao@xxxxxxxxx> 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@xxxxxxxxxxxxx https://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |