Re: [RFC PATCH] vfio/pci: Use kernel VPD access functions

[Rustad, Mark D]

> On Sep 11, 2015, at 6:11 PM, Rustad, Mark D <mark.d.rus...@intel.com> wrote:
> 
> Superficially this looks pretty good. I need to think harder to be sure of 
> the details.

This is the first time I've looked at all at any of the vfio code, but this is 
still looking good to me. Thanks for taking this on and exposing the vfio code 
to me. I hope more devices will be able to take advantage of the quirk and get 
their VPD issues resolved.

I did run this on a host with a device with VPD assigned to a guest and did not 
see any trouble when accessing it concurrently from both the guest and the host 
on the same and different functions. I don't think my particular environment is 
ideal to fully reproduce the problem (no writable VPD area), but my initial 
testing looks good.

Acked-by: Mark Rustad <mark.d.rus...@intel.com>

--
Mark Rustad, Networking Division, Intel Corporation



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [RFC PATCH] vfio/pci: Use kernel VPD access functions

[Rustad, Mark D]

Alex,

> On Sep 11, 2015, at 11:16 AM, Alex Williamson  
> wrote:
> 
> RFC - Is this something we should do?

Superficially this looks pretty good. I need to think harder to be sure of the 
details.

> Should we consider providing
> similar emulation through PCI sysfs to allow lspci to also make use
> of the vpd interfaces?

It looks to me like lspci already uses the vpd attribute in sysfs to access 
VPD, so maybe nothing more than this is needed. No doubt lspci can be coerced 
into accessing VPD directly, but is that really worth going after? I'm not so 
sure.

An strace of lspci accessing a device with VPD shows me:

write(1, "\tCapabilities: [e0] Vital Produc"..., 39 Capabilities: [e0] 
Vital Product Data
) = 39
open("/sys/bus/pci/devices/:02:00.0/vpd", O_RDONLY) = 4
^^^ accesses to this should be safe, I 
think

pread(4, "\202", 1, 0)  = 1
pread(4, "\10\0", 2, 1) = 2
pread(4, "PVL Dell", 8, 3)  = 8
write(1, "\t\tProduct Name: PVL Dell\n", 25 Product Name: PVL Dell
) = 25

and so forth.

--
Mark Rustad, Networking Division, Intel Corporation



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH V2 1/4] x86/kvm: Resolve some missing-initializers warnings

[Rustad, Mark D]

On Jul 31, 2014, at 4:50 AM, Paolo Bonzini pbonz...@redhat.com wrote:

 Il 30/07/2014 23:18, Mark D Rustad ha scritto:
 Resolve some missing-initializers warnings that appear in W=2
 builds. They are resolved by adding the name as a parameter to
 the macros and having the macro generate all four fields of the
 structure.
 
 Signed-off-by: Mark Rustad mark.d.rus...@intel.com
 Signed-off-by: Jeff Kirsher jeffrey.t.kirs...@intel.com
 
 ---
 V2: Change macro to supply all four fields instead of using a
designated initializer. Also fix up the array terminator.
 ---
 arch/x86/kvm/x86.c |   71 
 ++--
 1 file changed, 36 insertions(+), 35 deletions(-)
 
 diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
 index ef432f891d30..623aea52ceba 100644
 --- a/arch/x86/kvm/x86.c
 +++ b/arch/x86/kvm/x86.c
 @@ -82,8 +82,9 @@ u64 __read_mostly efer_reserved_bits = ~((u64)(EFER_SCE | 
 EFER_LME | EFER_LMA));
 static u64 __read_mostly efer_reserved_bits = ~((u64)EFER_SCE);
 #endif
 
 -#define VM_STAT(x) offsetof(struct kvm, stat.x), KVM_STAT_VM
 -#define VCPU_STAT(x) offsetof(struct kvm_vcpu, stat.x), KVM_STAT_VCPU
 +#define VM_STAT(name, x) name, offsetof(struct kvm, stat.x), KVM_STAT_VM, 
 NULL
 +#define VCPU_STAT(name, x) name, offsetof(struct kvm_vcpu, stat.x), \
 +   KVM_STAT_VCPU, NULL
 
 static void update_cr8_intercept(struct kvm_vcpu *vcpu);
 static void process_nmi(struct kvm_vcpu *vcpu);
 @@ -128,39 +129,39 @@ static struct kvm_shared_msrs_global __read_mostly 
 shared_msrs_global;
 static struct kvm_shared_msrs __percpu *shared_msrs;
 
 struct kvm_stats_debugfs_item debugfs_entries[] = {
 -{ pf_fixed, VCPU_STAT(pf_fixed) },
 -{ pf_guest, VCPU_STAT(pf_guest) },
 -{ tlb_flush, VCPU_STAT(tlb_flush) },
 -{ invlpg, VCPU_STAT(invlpg) },
 -{ exits, VCPU_STAT(exits) },
 -{ io_exits, VCPU_STAT(io_exits) },
 -{ mmio_exits, VCPU_STAT(mmio_exits) },
 -{ signal_exits, VCPU_STAT(signal_exits) },
 -{ irq_window, VCPU_STAT(irq_window_exits) },
 -{ nmi_window, VCPU_STAT(nmi_window_exits) },
 -{ halt_exits, VCPU_STAT(halt_exits) },
 -{ halt_wakeup, VCPU_STAT(halt_wakeup) },
 -{ hypercalls, VCPU_STAT(hypercalls) },
 -{ request_irq, VCPU_STAT(request_irq_exits) },
 -{ irq_exits, VCPU_STAT(irq_exits) },
 -{ host_state_reload, VCPU_STAT(host_state_reload) },
 -{ efer_reload, VCPU_STAT(efer_reload) },
 -{ fpu_reload, VCPU_STAT(fpu_reload) },
 -{ insn_emulation, VCPU_STAT(insn_emulation) },
 -{ insn_emulation_fail, VCPU_STAT(insn_emulation_fail) },
 -{ irq_injections, VCPU_STAT(irq_injections) },
 -{ nmi_injections, VCPU_STAT(nmi_injections) },
 -{ mmu_shadow_zapped, VM_STAT(mmu_shadow_zapped) },
 -{ mmu_pte_write, VM_STAT(mmu_pte_write) },
 -{ mmu_pte_updated, VM_STAT(mmu_pte_updated) },
 -{ mmu_pde_zapped, VM_STAT(mmu_pde_zapped) },
 -{ mmu_flooded, VM_STAT(mmu_flooded) },
 -{ mmu_recycled, VM_STAT(mmu_recycled) },
 -{ mmu_cache_miss, VM_STAT(mmu_cache_miss) },
 -{ mmu_unsync, VM_STAT(mmu_unsync) },
 -{ remote_tlb_flush, VM_STAT(remote_tlb_flush) },
 -{ largepages, VM_STAT(lpages) },
 -{ NULL }
 +{ VCPU_STAT(pf_fixed, pf_fixed) },
 +{ VCPU_STAT(pf_guest, pf_guest) },
 +{ VCPU_STAT(tlb_flush, tlb_flush) },
 +{ VCPU_STAT(invlpg, invlpg) },
 +{ VCPU_STAT(exits, exits) },
 +{ VCPU_STAT(io_exits, io_exits) },
 +{ VCPU_STAT(mmio_exits, mmio_exits) },
 +{ VCPU_STAT(signal_exits, signal_exits) },
 +{ VCPU_STAT(irq_window, irq_window_exits) },
 +{ VCPU_STAT(nmi_window, nmi_window_exits) },
 +{ VCPU_STAT(halt_exits, halt_exits) },
 +{ VCPU_STAT(halt_wakeup, halt_wakeup) },
 +{ VCPU_STAT(hypercalls, hypercalls) },
 +{ VCPU_STAT(request_irq, request_irq_exits) },
 +{ VCPU_STAT(irq_exits, irq_exits) },
 +{ VCPU_STAT(host_state_reload, host_state_reload) },
 +{ VCPU_STAT(efer_reload, efer_reload) },
 +{ VCPU_STAT(fpu_reload, fpu_reload) },
 +{ VCPU_STAT(insn_emulation, insn_emulation) },
 +{ VCPU_STAT(insn_emulation_fail, insn_emulation_fail) },
 +{ VCPU_STAT(irq_injections, irq_injections) },
 +{ VCPU_STAT(nmi_injections, nmi_injections) },
 +{ VM_STAT(mmu_shadow_zapped, mmu_shadow_zapped) },
 +{ VM_STAT(mmu_pte_write, mmu_pte_write) },
 +{ VM_STAT(mmu_pte_updated, mmu_pte_updated) },
 +{ VM_STAT(mmu_pde_zapped, mmu_pde_zapped) },
 +{ VM_STAT(mmu_flooded, mmu_flooded) },
 +{ VM_STAT(mmu_recycled, mmu_recycled) },
 +{ VM_STAT(mmu_cache_miss, mmu_cache_miss) },
 +{ VM_STAT(mmu_unsync, mmu_unsync) },
 +{ VM_STAT(remote_tlb_flush, remote_tlb_flush) },
 +{ VM_STAT(largepages, lpages) },
 
 Please move the braces inside the macro as well.

I should have thought of that. I will do that in a new version. That would be 
much better.

 +{ NULL, 0, 0, NULL }
 
 This is ugly.  Do you really mind having one residual warning? :)

I 

Re: [PATCH 3/4] x86/kvm: Resolve shadow warnings in macro expansion

[Rustad, Mark D]

On Jul 25, 2014, at 7:06 AM, Paolo Bonzini pbonz...@redhat.com wrote:

 Il 25/07/2014 15:27, Jeff Kirsher ha scritto:
 From: Mark Rustad mark.d.rus...@intel.com
 
 Resolve shadow warnings that appear in W=2 builds. In this case,
 a macro declared an inner local variable with the same name as an
 outer one. This can be a serious hazard in the event that the outer
 variable is ever passed as a parameter, as the inner variable will
 be referenced instead of what was intended. This macro doesn't have
 any parameters - at this time - but prepend an _ to all of the
 macro-declared variables as is the custom, to resolve the warnings
 and eliminate any future hazard.
 
 Signed-off-by: Mark Rustad mark.d.rus...@intel.com
 Signed-off-by: Jeff Kirsher jeffrey.t.kirs...@intel.com
 ---
 arch/x86/kvm/mmutrace.h | 24 
 1 file changed, 12 insertions(+), 12 deletions(-)
 
 diff --git a/arch/x86/kvm/mmutrace.h b/arch/x86/kvm/mmutrace.h
 index 9d2e0ff..2d8d00c 100644
 --- a/arch/x86/kvm/mmutrace.h
 +++ b/arch/x86/kvm/mmutrace.h
 @@ -22,26 +22,26 @@
  __entry-unsync = sp-unsync;
 
 #define KVM_MMU_PAGE_PRINTK() ({ \
 -const char *ret = p-buffer + p-len;   \
 -static const char *access_str[] = { \
 +const char *_ret = p-buffer + p-len;  \
 +static const char *_access_str[] = {\
  ---, --x, w--, w-x, -u-, -ux, wu-, wux  \
  };  \
 -union kvm_mmu_page_role role;   \
 +union kvm_mmu_page_role _role;  \
  \
 -role.word = __entry-role;  \
 +_role.word = __entry-role; \
  \
  trace_seq_printf(p, sp gen %lx gfn %llx %u%s q%u%s %s%s   \
%snxe root %u %s%c, __entry-mmu_valid_gen, \
 - __entry-gfn, role.level,  \
 - role.cr4_pae ?  pae : ,\
 - role.quadrant, \
 - role.direct ?  direct : ,  \
 - access_str[role.access],   \
 - role.invalid ?  invalid : ,\
 - role.nxe ?  : !,   \
 + __entry-gfn, _role.level, \
 + _role.cr4_pae ?  pae : ,   \
 + _role.quadrant,\
 + _role.direct ?  direct : , \
 + _access_str[_role.access], \
 + _role.invalid ?  invalid : ,   \
 + _role.nxe ?  : !,  \
   __entry-root_count,   \
   __entry-unsync ? unsync : sync, 0);   \
 -ret;\
 +_ret;   \
  })
 
 #define kvm_mmu_trace_pferr_flags   \
 
 
 I think this unnecessarily uglifies the code, so I am not applying it.
 Gleb, what do you think?

Would you consider a version that only changes ret to _ret? That would be 
enough to resolve the warning. I only did the other variables because it seemed 
to be a best practice for these inner block declarations.

Hmmm. It seems like p should really be a parameter to this macro.

-- 
Mark Rustad, Networking Division, Intel Corporation

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC-v2 0/4] tcm_vhost+cmwq fabric driver code for-3.6

[Rustad, Mark D]

On Jul 18, 2012, at 9:00 AM, Michael S. Tsirkin wrote:

 On Wed, Jul 18, 2012 at 11:53:38AM -0400, Christoph Hellwig wrote:
 On Wed, Jul 18, 2012 at 08:42:21AM -0500, Anthony Liguori wrote:
 
 If you add support for a new command, you need to provide userspace
 a way to disable this command.  If you change what gets reported for
 VPD, you need to provide userspace a way to make VPD look like what
 it did in a previous version.
 
 Basically, you need to be able to make a TCM device behave 100% the
 same as it did in an older version of the kernel.
 
 This is unique to virtualization due to live migration.  If you
 migrate from a 3.6 kernel to a 3.8 kernel, you need to make sure
 that the 3.8 kernel's TCM device behaves exactly like the 3.6 kernel
 because the guest that is interacting with it does not realize that
 live migration happened.
 
 I don't think these strict live migration rules apply to SCSI targets.
 
 Real life storage systems get new features and different behaviour with
 firmware upgrades all the time, and SCSI initiators deal with that just
 fine.
 I don't see any reason to be more picky just because we're
 virtualized.
 
 Presumably initiators are shut down for target firmware upgrades?
 With virtualization your host can change without guest shutdown.
 You can also *lose* commands when migrating to an older host.


Actually no. Storage vendors do not want to impose a need to take initiators 
down for any reason. I have worked for a storage system vendor that routinely 
did firmware upgrades on-the-fly. This is done by multi-pathing and taking one 
path down, upgrade, bring up, repeat. There was even one non-redundant system 
that I am aware of that could upgrade firmware and reboot fast enough that the 
initiators would not notice.

You do have to pay very close attention to some things however. Don't change 
the device identity in any way - even version information, otherwise a Windows 
initiator will blue-screen. I made that mistake myself, so I remember it well. 
It seemed like such an innocent change. I don't recall there being any issue 
with adding commands and we did do that on occasion.

-- 
Mark Rustad, LAN Access Division, Intel Corporation

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC-v2 0/4] tcm_vhost+cmwq fabric driver code for-3.6

[Rustad, Mark D]

On Jul 18, 2012, at 10:17 AM, Michael S. Tsirkin wrote:

snip

 You do have to pay very close attention to some things however. Don't change 
 the device identity in any way - even version information, otherwise a 
 Windows initiator will blue-screen. I made that mistake myself, so I 
 remember it well. It seemed like such an innocent change. I don't recall 
 there being any issue with adding commands and we did do that on occasion.
 
 How about removing commands?

Good question. With the storage system I am familiar with, that would only be a 
risk if firmware were downgraded. Downgrading would never have been 
recommended. I am sure that if something like persistent reserve suddenly went 
away it would cause big trouble for some initiators.

-- 
Mark Rustad, LAN Access Division, Intel Corporation

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html