On Mon, Jan 16, 2017 at 04:32:24PM +0800, Peter Xu wrote: > On Mon, Jan 16, 2017 at 04:25:35PM +0800, Jason Wang wrote: > > > > > > On 2017年01月16日 16:12, Peter Xu wrote: > > >On Mon, Jan 16, 2017 at 04:01:00PM +0800, Jason Wang wrote: > > >> > > >>On 2017年01月16日 15:50, Peter Xu wrote: > > >>>On Mon, Jan 16, 2017 at 02:20:31PM +0800, Jason Wang wrote: > > >>> > > >>>[...] > > >>> > > >>>>>diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > > >>>>>index fd75112..2596f11 100644 > > >>>>>--- a/hw/i386/intel_iommu.c > > >>>>>+++ b/hw/i386/intel_iommu.c > > >>>>>@@ -1343,9 +1343,49 @@ static void > > >>>>>vtd_handle_gcmd_sirtp(IntelIOMMUState *s) > > >>>>> vtd_set_clear_mask_long(s, DMAR_GSTS_REG, 0, VTD_GSTS_IRTPS); > > >>>>> } > > >>>>>+static void vtd_switch_address_space(VTDAddressSpace *as, bool > > >>>>>iommu_enabled) > > >>>>Looks like you can check s->dmar_enabled here? > > >>>Yes, we need to check old state in case we don't need a switch at all. > > >>>Actually I checked it... > > >>> > > >>I mean is there a chance that iommu_enabled( better name should be > > >>dmar_enabled) is not equal to s->dmar_enabled? Looks not. > > >> > > >>vtd_handle_gcmd_te() did: > > >> > > >> ... > > >> if (en) { > > >> s->dmar_enabled = true; > > >> /* Ok - report back to driver */ > > >> vtd_set_clear_mask_long(s, DMAR_GSTS_REG, 0, VTD_GSTS_TES); > > >> } else { > > >> s->dmar_enabled = false; > > >> ... > > >> > > >>You can vtd_switch_address_space_all(s, en) after this which will call > > >>this > > >>function. And another caller like you've pointed out has already call this > > >>through s->dmar_enabled. So en here is always s->dmar_enalbed? > > >Hmm, yes... > > > > > >(I would still prefer keeping this parameter for readablility. > > > Though, I prefer your suggestion to rename it to dmar_enabled) > > > > > >-- peterx > > > > I think this does not give more readability :) May I was wrong, let leave > > this for maintainer. > > > > Thanks :) > > Thanks for reviewing this series so fast! > > I have no strong opinion as well. Maybe you are right. :-) > > Michael, please let me know if you dislike this, so I can remove this > parameter (it equals to as->iommu_state->dmar_enabled). > > Thanks, > > -- peterx
I prefer not to duplicate data, yes.