Re: [Xen-devel] [PATCH v3 03/14] AMD/IOMMU: use bit field for control register
On 19.07.2019 20:23, Woods, Brian wrote: > On Tue, Jul 16, 2019 at 04:36:06PM +, Jan Beulich wrote: >> Also introduce a field in struct amd_iommu caching the most recently >> written control register. All writes should now happen exclusively from >> that cached value, such that it is guaranteed to be up to date. >> >> Take the opportunity and add further fields. Also convert a few boolean >> function parameters to bool, such that use of !! can be avoided. >> >> Because of there now being definitions beyond bit 31, writel() also gets >> replaced by writeq() when updating hardware. >> >> Signed-off-by: Jan Beulich >> Acked-by: Andrew Cooper > > Acked-by: Brian Woods Thanks for this and the other acks. I notice though that you skipped patches 2 and 13: Are there concerns there? Patch 8 still has a discussion to settle, so I realize you probably wouldn't want to ack that one yet. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3 03/14] AMD/IOMMU: use bit field for control register
On Tue, Jul 16, 2019 at 04:36:06PM +, Jan Beulich wrote:
> Also introduce a field in struct amd_iommu caching the most recently
> written control register. All writes should now happen exclusively from
> that cached value, such that it is guaranteed to be up to date.
>
> Take the opportunity and add further fields. Also convert a few boolean
> function parameters to bool, such that use of !! can be avoided.
>
> Because of there now being definitions beyond bit 31, writel() also gets
> replaced by writeq() when updating hardware.
>
> Signed-off-by: Jan Beulich
> Acked-by: Andrew Cooper
Acked-by: Brian Woods
> ---
> v3: Switch boolean bitfields to bool.
> v2: Add domain_id_pne field. Mention writel() -> writeq() change.
>
> --- a/xen/drivers/passthrough/amd/iommu_guest.c
> +++ b/xen/drivers/passthrough/amd/iommu_guest.c
> @@ -317,7 +317,7 @@ static int do_invalidate_iotlb_pages(str
>
> static int do_completion_wait(struct domain *d, cmd_entry_t *cmd)
> {
> -bool_t com_wait_int_en, com_wait_int, i, s;
> +bool com_wait_int, i, s;
> struct guest_iommu *iommu;
> unsigned long gfn;
> p2m_type_t p2mt;
> @@ -354,12 +354,10 @@ static int do_completion_wait(struct dom
> unmap_domain_page(vaddr);
> }
>
> -com_wait_int_en = iommu_get_bit(iommu->reg_ctrl.lo,
> -IOMMU_CONTROL_COMP_WAIT_INT_SHIFT);
> com_wait_int = iommu_get_bit(iommu->reg_status.lo,
>IOMMU_STATUS_COMP_WAIT_INT_SHIFT);
>
> -if ( com_wait_int_en && com_wait_int )
> +if ( iommu->reg_ctrl.com_wait_int_en && com_wait_int )
> guest_iommu_deliver_msi(d);
>
> return 0;
> @@ -521,40 +519,17 @@ static void guest_iommu_process_command(
> return;
> }
>
> -static int guest_iommu_write_ctrl(struct guest_iommu *iommu, uint64_t
> newctrl)
> +static int guest_iommu_write_ctrl(struct guest_iommu *iommu, uint64_t val)
> {
> -bool_t cmd_en, event_en, iommu_en, ppr_en, ppr_log_en;
> -bool_t cmd_en_old, event_en_old, iommu_en_old;
> -bool_t cmd_run;
> -
> -iommu_en = iommu_get_bit(newctrl,
> - IOMMU_CONTROL_TRANSLATION_ENABLE_SHIFT);
> -iommu_en_old = iommu_get_bit(iommu->reg_ctrl.lo,
> - IOMMU_CONTROL_TRANSLATION_ENABLE_SHIFT);
> -
> -cmd_en = iommu_get_bit(newctrl,
> - IOMMU_CONTROL_COMMAND_BUFFER_ENABLE_SHIFT);
> -cmd_en_old = iommu_get_bit(iommu->reg_ctrl.lo,
> - IOMMU_CONTROL_COMMAND_BUFFER_ENABLE_SHIFT);
> -cmd_run = iommu_get_bit(iommu->reg_status.lo,
> -IOMMU_STATUS_CMD_BUFFER_RUN_SHIFT);
> -event_en = iommu_get_bit(newctrl,
> - IOMMU_CONTROL_EVENT_LOG_ENABLE_SHIFT);
> -event_en_old = iommu_get_bit(iommu->reg_ctrl.lo,
> - IOMMU_CONTROL_EVENT_LOG_ENABLE_SHIFT);
> -
> -ppr_en = iommu_get_bit(newctrl,
> - IOMMU_CONTROL_PPR_ENABLE_SHIFT);
> -ppr_log_en = iommu_get_bit(newctrl,
> - IOMMU_CONTROL_PPR_LOG_ENABLE_SHIFT);
> +union amd_iommu_control newctrl = { .raw = val };
>
> -if ( iommu_en )
> +if ( newctrl.iommu_en )
> {
> guest_iommu_enable(iommu);
> guest_iommu_enable_dev_table(iommu);
> }
>
> -if ( iommu_en && cmd_en )
> +if ( newctrl.iommu_en && newctrl.cmd_buf_en )
> {
> guest_iommu_enable_ring_buffer(iommu, >cmd_buffer,
> sizeof(cmd_entry_t));
> @@ -562,7 +537,7 @@ static int guest_iommu_write_ctrl(struct
> tasklet_schedule(>cmd_buffer_tasklet);
> }
>
> -if ( iommu_en && event_en )
> +if ( newctrl.iommu_en && newctrl.event_log_en )
> {
> guest_iommu_enable_ring_buffer(iommu, >event_log,
> sizeof(event_entry_t));
> @@ -570,7 +545,7 @@ static int guest_iommu_write_ctrl(struct
> guest_iommu_clear_status(iommu, IOMMU_STATUS_EVENT_OVERFLOW_SHIFT);
> }
>
> -if ( iommu_en && ppr_en && ppr_log_en )
> +if ( newctrl.iommu_en && newctrl.ppr_en && newctrl.ppr_log_en )
> {
> guest_iommu_enable_ring_buffer(iommu, >ppr_log,
> sizeof(ppr_entry_t));
> @@ -578,19 +553,21 @@ static int guest_iommu_write_ctrl(struct
> guest_iommu_clear_status(iommu,
> IOMMU_STATUS_PPR_LOG_OVERFLOW_SHIFT);
> }
>
> -if ( iommu_en && cmd_en_old && !cmd_en )
> +if ( newctrl.iommu_en && iommu->reg_ctrl.cmd_buf_en &&
> + !newctrl.cmd_buf_en )
> {
> /* Disable iommu command processing */
> tasklet_kill(>cmd_buffer_tasklet);
> }
>
> -if ( event_en_old && !event_en )
> +if ( iommu->reg_ctrl.event_log_en && !newctrl.event_log_en )
>
[Xen-devel] [PATCH v3 03/14] AMD/IOMMU: use bit field for control register
Also introduce a field in struct amd_iommu caching the most recently
written control register. All writes should now happen exclusively from
that cached value, such that it is guaranteed to be up to date.
Take the opportunity and add further fields. Also convert a few boolean
function parameters to bool, such that use of !! can be avoided.
Because of there now being definitions beyond bit 31, writel() also gets
replaced by writeq() when updating hardware.
Signed-off-by: Jan Beulich
Acked-by: Andrew Cooper
---
v3: Switch boolean bitfields to bool.
v2: Add domain_id_pne field. Mention writel() -> writeq() change.
--- a/xen/drivers/passthrough/amd/iommu_guest.c
+++ b/xen/drivers/passthrough/amd/iommu_guest.c
@@ -317,7 +317,7 @@ static int do_invalidate_iotlb_pages(str
static int do_completion_wait(struct domain *d, cmd_entry_t *cmd)
{
-bool_t com_wait_int_en, com_wait_int, i, s;
+bool com_wait_int, i, s;
struct guest_iommu *iommu;
unsigned long gfn;
p2m_type_t p2mt;
@@ -354,12 +354,10 @@ static int do_completion_wait(struct dom
unmap_domain_page(vaddr);
}
-com_wait_int_en = iommu_get_bit(iommu->reg_ctrl.lo,
-IOMMU_CONTROL_COMP_WAIT_INT_SHIFT);
com_wait_int = iommu_get_bit(iommu->reg_status.lo,
IOMMU_STATUS_COMP_WAIT_INT_SHIFT);
-if ( com_wait_int_en && com_wait_int )
+if ( iommu->reg_ctrl.com_wait_int_en && com_wait_int )
guest_iommu_deliver_msi(d);
return 0;
@@ -521,40 +519,17 @@ static void guest_iommu_process_command(
return;
}
-static int guest_iommu_write_ctrl(struct guest_iommu *iommu, uint64_t newctrl)
+static int guest_iommu_write_ctrl(struct guest_iommu *iommu, uint64_t val)
{
-bool_t cmd_en, event_en, iommu_en, ppr_en, ppr_log_en;
-bool_t cmd_en_old, event_en_old, iommu_en_old;
-bool_t cmd_run;
-
-iommu_en = iommu_get_bit(newctrl,
- IOMMU_CONTROL_TRANSLATION_ENABLE_SHIFT);
-iommu_en_old = iommu_get_bit(iommu->reg_ctrl.lo,
- IOMMU_CONTROL_TRANSLATION_ENABLE_SHIFT);
-
-cmd_en = iommu_get_bit(newctrl,
- IOMMU_CONTROL_COMMAND_BUFFER_ENABLE_SHIFT);
-cmd_en_old = iommu_get_bit(iommu->reg_ctrl.lo,
- IOMMU_CONTROL_COMMAND_BUFFER_ENABLE_SHIFT);
-cmd_run = iommu_get_bit(iommu->reg_status.lo,
-IOMMU_STATUS_CMD_BUFFER_RUN_SHIFT);
-event_en = iommu_get_bit(newctrl,
- IOMMU_CONTROL_EVENT_LOG_ENABLE_SHIFT);
-event_en_old = iommu_get_bit(iommu->reg_ctrl.lo,
- IOMMU_CONTROL_EVENT_LOG_ENABLE_SHIFT);
-
-ppr_en = iommu_get_bit(newctrl,
- IOMMU_CONTROL_PPR_ENABLE_SHIFT);
-ppr_log_en = iommu_get_bit(newctrl,
- IOMMU_CONTROL_PPR_LOG_ENABLE_SHIFT);
+union amd_iommu_control newctrl = { .raw = val };
-if ( iommu_en )
+if ( newctrl.iommu_en )
{
guest_iommu_enable(iommu);
guest_iommu_enable_dev_table(iommu);
}
-if ( iommu_en && cmd_en )
+if ( newctrl.iommu_en && newctrl.cmd_buf_en )
{
guest_iommu_enable_ring_buffer(iommu, >cmd_buffer,
sizeof(cmd_entry_t));
@@ -562,7 +537,7 @@ static int guest_iommu_write_ctrl(struct
tasklet_schedule(>cmd_buffer_tasklet);
}
-if ( iommu_en && event_en )
+if ( newctrl.iommu_en && newctrl.event_log_en )
{
guest_iommu_enable_ring_buffer(iommu, >event_log,
sizeof(event_entry_t));
@@ -570,7 +545,7 @@ static int guest_iommu_write_ctrl(struct
guest_iommu_clear_status(iommu, IOMMU_STATUS_EVENT_OVERFLOW_SHIFT);
}
-if ( iommu_en && ppr_en && ppr_log_en )
+if ( newctrl.iommu_en && newctrl.ppr_en && newctrl.ppr_log_en )
{
guest_iommu_enable_ring_buffer(iommu, >ppr_log,
sizeof(ppr_entry_t));
@@ -578,19 +553,21 @@ static int guest_iommu_write_ctrl(struct
guest_iommu_clear_status(iommu, IOMMU_STATUS_PPR_LOG_OVERFLOW_SHIFT);
}
-if ( iommu_en && cmd_en_old && !cmd_en )
+if ( newctrl.iommu_en && iommu->reg_ctrl.cmd_buf_en &&
+ !newctrl.cmd_buf_en )
{
/* Disable iommu command processing */
tasklet_kill(>cmd_buffer_tasklet);
}
-if ( event_en_old && !event_en )
+if ( iommu->reg_ctrl.event_log_en && !newctrl.event_log_en )
guest_iommu_clear_status(iommu, IOMMU_STATUS_EVENT_LOG_RUN_SHIFT);
-if ( iommu_en_old && !iommu_en )
+if ( iommu->reg_ctrl.iommu_en && !newctrl.iommu_en )
guest_iommu_disable(iommu);
-u64_to_reg(>reg_ctrl, newctrl);
+iommu->reg_ctrl = newctrl;
+
return 0;
}
@@ -632,7 +609,7 @@ static uint64_t
