| Message ID | b143bc0c-3d13-2127-be5d-b459d7b53c1e@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | AMD IOMMU: further improvements | expand |
On 26/09/2019 15:29, Jan Beulich wrote: > The command ring buffer doesn't need clearing up front in any event. > Subsequently we'll also want to avoid clearing the device tables. > > While playing with functions signatures replace undue use of fixed width > types at the same time, and extend this to deallocate_buffer() as well. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > v7: New. > > --- a/xen/drivers/passthrough/amd/iommu_init.c > +++ b/xen/drivers/passthrough/amd/iommu_init.c > @@ -994,12 +994,12 @@ static unsigned int __init dt_alloc_size > IOMMU_DEV_TABLE_ENTRY_SIZE); > } > > -static void __init deallocate_buffer(void *buf, uint32_t sz) > +static void __init deallocate_buffer(void *buf, unsigned long sz) Probably best to use size_t here, being both shorter, and guaranteed not to require modification in the future. > { > - int order = 0; > if ( buf ) > { > - order = get_order_from_bytes(sz); > + unsigned int order = get_order_from_bytes(sz); > + > __free_amd_iommu_tables(buf, order); > } > } How about simply if ( buf ) __free_amd_iommu_tables(buf, get_order_from_bytes(sz)); which drops the order variable entirely? Ideally with both of these modifications, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> > @@ -1050,21 +1055,23 @@ static void * __init allocate_cmd_buffer > /* allocate 'command buffer' in power of 2 increments of 4K */ > return allocate_ring_buffer(&iommu->cmd_buffer, sizeof(cmd_entry_t), > IOMMU_CMD_BUFFER_DEFAULT_ENTRIES, > - "Command Buffer"); > + "Command Buffer", false); > } > > static void * __init allocate_event_log(struct amd_iommu *iommu) > { > /* allocate 'event log' in power of 2 increments of 4K */ > return allocate_ring_buffer(&iommu->event_log, sizeof(event_entry_t), > - IOMMU_EVENT_LOG_DEFAULT_ENTRIES, "Event Log"); > + IOMMU_EVENT_LOG_DEFAULT_ENTRIES, "Event Log", > + true); Well - this means I've got yet another AMD IOMMU bugfix hiding somewhere in a branch. I could have sworn I posted it. ~Andrew
On 04.10.2019 15:26, Andrew Cooper wrote: > On 26/09/2019 15:29, Jan Beulich wrote: >> The command ring buffer doesn't need clearing up front in any event. >> Subsequently we'll also want to avoid clearing the device tables. >> >> While playing with functions signatures replace undue use of fixed width >> types at the same time, and extend this to deallocate_buffer() as well. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> --- >> v7: New. >> >> --- a/xen/drivers/passthrough/amd/iommu_init.c >> +++ b/xen/drivers/passthrough/amd/iommu_init.c >> @@ -994,12 +994,12 @@ static unsigned int __init dt_alloc_size >> IOMMU_DEV_TABLE_ENTRY_SIZE); >> } >> >> -static void __init deallocate_buffer(void *buf, uint32_t sz) >> +static void __init deallocate_buffer(void *buf, unsigned long sz) > > Probably best to use size_t here, being both shorter, and guaranteed not > to require modification in the future. I'd prefer not to without other related code also getting switched from unsigned long to size_t. >> { >> - int order = 0; >> if ( buf ) >> { >> - order = get_order_from_bytes(sz); >> + unsigned int order = get_order_from_bytes(sz); >> + >> __free_amd_iommu_tables(buf, order); >> } >> } > > How about simply > > if ( buf ) > __free_amd_iommu_tables(buf, get_order_from_bytes(sz)); > > which drops the order variable entirely? Fine with me; I did actually consider doing so, but then decided against to stay closer to what the code looked like before. > Ideally with both of these modifications, Reviewed-by: Andrew Cooper > <andrew.cooper3@citrix.com> Thanks. Jan
--- a/xen/drivers/passthrough/amd/iommu_init.c +++ b/xen/drivers/passthrough/amd/iommu_init.c @@ -994,12 +994,12 @@ static unsigned int __init dt_alloc_size IOMMU_DEV_TABLE_ENTRY_SIZE); } -static void __init deallocate_buffer(void *buf, uint32_t sz) +static void __init deallocate_buffer(void *buf, unsigned long sz) { - int order = 0; if ( buf ) { - order = get_order_from_bytes(sz); + unsigned int order = get_order_from_bytes(sz); + __free_amd_iommu_tables(buf, order); } } @@ -1012,10 +1012,11 @@ static void __init deallocate_ring_buffe ring_buf->tail = 0; } -static void * __init allocate_buffer(uint32_t alloc_size, const char *name) +static void *__init allocate_buffer(unsigned long alloc_size, + const char *name, bool clear) { - void * buffer; - int order = get_order_from_bytes(alloc_size); + void *buffer; + unsigned int order = get_order_from_bytes(alloc_size); buffer = __alloc_amd_iommu_tables(order); @@ -1025,13 +1026,16 @@ static void * __init allocate_buffer(uin return NULL; } - memset(buffer, 0, PAGE_SIZE * (1UL << order)); + if ( clear ) + memset(buffer, 0, PAGE_SIZE << order); + return buffer; } -static void * __init allocate_ring_buffer(struct ring_buffer *ring_buf, - uint32_t entry_size, - uint64_t entries, const char *name) +static void *__init allocate_ring_buffer(struct ring_buffer *ring_buf, + unsigned int entry_size, + unsigned long entries, + const char *name, bool clear) { ring_buf->head = 0; ring_buf->tail = 0; @@ -1041,7 +1045,8 @@ static void * __init allocate_ring_buffe ring_buf->alloc_size = PAGE_SIZE << get_order_from_bytes(entries * entry_size); ring_buf->entries = ring_buf->alloc_size / entry_size; - ring_buf->buffer = allocate_buffer(ring_buf->alloc_size, name); + ring_buf->buffer = allocate_buffer(ring_buf->alloc_size, name, clear); + return ring_buf->buffer; } @@ -1050,21 +1055,23 @@ static void * __init allocate_cmd_buffer /* allocate 'command buffer' in power of 2 increments of 4K */ return allocate_ring_buffer(&iommu->cmd_buffer, sizeof(cmd_entry_t), IOMMU_CMD_BUFFER_DEFAULT_ENTRIES, - "Command Buffer"); + "Command Buffer", false); } static void * __init allocate_event_log(struct amd_iommu *iommu) { /* allocate 'event log' in power of 2 increments of 4K */ return allocate_ring_buffer(&iommu->event_log, sizeof(event_entry_t), - IOMMU_EVENT_LOG_DEFAULT_ENTRIES, "Event Log"); + IOMMU_EVENT_LOG_DEFAULT_ENTRIES, "Event Log", + true); } static void * __init allocate_ppr_log(struct amd_iommu *iommu) { /* allocate 'ppr log' in power of 2 increments of 4K */ return allocate_ring_buffer(&iommu->ppr_log, sizeof(ppr_entry_t), - IOMMU_PPR_LOG_DEFAULT_ENTRIES, "PPR Log"); + IOMMU_PPR_LOG_DEFAULT_ENTRIES, "PPR Log", + true); } /* @@ -1257,7 +1264,7 @@ static int __init amd_iommu_setup_device { /* allocate 'device table' on a 4K boundary */ dt = IVRS_MAPPINGS_DEVTAB(ivrs_mappings) = - allocate_buffer(dt_alloc_size(), "Device Table"); + allocate_buffer(dt_alloc_size(), "Device Table", true); } if ( !dt ) return -ENOMEM;
The command ring buffer doesn't need clearing up front in any event. Subsequently we'll also want to avoid clearing the device tables. While playing with functions signatures replace undue use of fixed width types at the same time, and extend this to deallocate_buffer() as well. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- v7: New.