Re: [Qemu-devel] [PATCH] nvdimm acpi: fix region format interface code
On 06/08/17 14:40 +0800, Xiao Guangrong wrote: > > > On 06/08/2017 01:06 PM, Haozhong Zhang wrote: > > On 06/08/17 11:07 +0800, Xiao Guangrong wrote: > > > > > > > > > On 06/07/2017 04:06 PM, Haozhong Zhang wrote: > > > > Per ACPI 6.2, section 5.2.25.6 and JEDEC Annex L Release 3, the > > > > current region format interface code 0x201 indicates the block > > > > addressed function interface 1, rather than a byte addressable > > > > interface. Fix it by using 0x301 which indicates the byte addressable > > > > no energy backed function interface 1. > > > > > > > > Signed-off-by: Haozhong Zhang > > > > --- > > > >hw/acpi/nvdimm.c | 7 --- > > > >1 file changed, 4 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c > > > > index 8e7d6ec034..b5734f5897 100644 > > > > --- a/hw/acpi/nvdimm.c > > > > +++ b/hw/acpi/nvdimm.c > > > > @@ -338,9 +338,10 @@ static void nvdimm_build_structure_dcr(GArray > > > > *structures, DeviceState *dev) > > > >nfit_dcr->revision_id = cpu_to_le16(1 /* Current Revision > > > > supported > > > > in ACPI 6.0 is 1. */); > > > >nfit_dcr->serial_number = cpu_to_le32(sn); > > > > -nfit_dcr->fic = cpu_to_le16(0x201 /* Format Interface Code. See > > > > Chapter > > > > - 2: NVDIMM Device Specific > > > > Method > > > > - (DSM) in DSM Spec Rev1.*/); > > > > +nfit_dcr->fic = cpu_to_le16(0x301 /* Format Interface Code: > > > > + Byte addressable, no energy > > > > backed. > > > > + See ACPI 6.2, sect 5.2.25.6 > > > > and > > > > + JEDEC Annex L Release 3. */); > > > > > > Shouldn't the 'no energy backend' indicator be set only for !dax disk? > > > > Above specs define RFIC for two classes of byte-addressable NVDIMM: > > 1. 0x10[0|1]: A function containing byte addressable persistent memory > >whose persistence is achieved through the use of DRAM, > >nonvolatile memory (e.g., Flash) and an energy source. > > Only the DRAM portion is addressable by the system. > > 2. 0x30[0|1]: A function containing byte addressable persistent > >memory. All of the persistent memory is addressable by > >the system. No external energy source is required. > > If the last bit is 0, then it's a proprietary interface. > > As both of them indicate persistent memory, it is okay to me. > > The FIC, 0x201, comes from the document of Intel DSM example, i have no idea > why > it uses 0x201. Maybe it is worth being fixed too. > I guess the reason is it's an example, e.g. "... describes an *example* _DSM interface for NVDIMM Device with Region Format Interface Code (RFIC) of 0x0201" at the beginning of Chapter 2. Haozhong
Re: [Qemu-devel] [PATCH] nvdimm acpi: fix region format interface code
On 06/08/2017 01:06 PM, Haozhong Zhang wrote: On 06/08/17 11:07 +0800, Xiao Guangrong wrote: On 06/07/2017 04:06 PM, Haozhong Zhang wrote: Per ACPI 6.2, section 5.2.25.6 and JEDEC Annex L Release 3, the current region format interface code 0x201 indicates the block addressed function interface 1, rather than a byte addressable interface. Fix it by using 0x301 which indicates the byte addressable no energy backed function interface 1. Signed-off-by: Haozhong Zhang --- hw/acpi/nvdimm.c | 7 --- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c index 8e7d6ec034..b5734f5897 100644 --- a/hw/acpi/nvdimm.c +++ b/hw/acpi/nvdimm.c @@ -338,9 +338,10 @@ static void nvdimm_build_structure_dcr(GArray *structures, DeviceState *dev) nfit_dcr->revision_id = cpu_to_le16(1 /* Current Revision supported in ACPI 6.0 is 1. */); nfit_dcr->serial_number = cpu_to_le32(sn); -nfit_dcr->fic = cpu_to_le16(0x201 /* Format Interface Code. See Chapter - 2: NVDIMM Device Specific Method - (DSM) in DSM Spec Rev1.*/); +nfit_dcr->fic = cpu_to_le16(0x301 /* Format Interface Code: + Byte addressable, no energy backed. + See ACPI 6.2, sect 5.2.25.6 and + JEDEC Annex L Release 3. */); Shouldn't the 'no energy backend' indicator be set only for !dax disk? Above specs define RFIC for two classes of byte-addressable NVDIMM: 1. 0x10[0|1]: A function containing byte addressable persistent memory whose persistence is achieved through the use of DRAM, nonvolatile memory (e.g., Flash) and an energy source. Only the DRAM portion is addressable by the system. 2. 0x30[0|1]: A function containing byte addressable persistent memory. All of the persistent memory is addressable by the system. No external energy source is required. If the last bit is 0, then it's a proprietary interface. As both of them indicate persistent memory, it is okay to me. The FIC, 0x201, comes from the document of Intel DSM example, i have no idea why it uses 0x201. Maybe it is worth being fixed too.
Re: [Qemu-devel] [PATCH v2 3/4] nvdimm: add a boolean option "restrict"
On 06/07/17 16:27 +0100, Stefan Hajnoczi wrote:
> On Tue, Jun 06, 2017 at 03:22:28PM +0800, Haozhong Zhang wrote:
> > If a vNVDIMM device is not backed by a DAX device and its "restrict"
> > option is enabled, bit 3 of state flags in its region mapping
> > structure will be set, in order to notify the guest of the lack of
> > write persistence guarantee. Once this bit is set, the guest OS may
> > mark the vNVDIMM device as read-only.
> >
> > This option is disabled by default for backwards compatibility. It's
> > recommended to enable for the formal usage.
>
> Good idea. I think the following is cleaner:
>
> DEFINE_PROP_ON_OFF_AUTO("readonly") on the 'nvdimm' device. The
> following states are available:
>
> * 'on' - ACPI_NFIT_MEM_NOT_ARMED is set
> * 'off' - ACPI_NFIT_MEM_NOT_ARMED is clear
> * 'auto' - ACPI_NFIT_MEM_NOT_ARMED set if backend is not persistent
>
> This new property defaults to 'auto'. Machine types older than
> pc-i440fx-2.10 and pc-q35-2.10 default to 'on'.
I think the the name "readonly" is not precise, because QEMU only sets
one bit and does not prevent guest writes. It's guest decision to
treat the vNVDIMM devices as read-only (e.g. Linux kernel).
We may use "unsafe-write" instead.
Haozhong
[Qemu-devel] [PATCH RFC] spapr: ignore interrupts during reset state
Rebooting a SMP TCG guest is broken for both single/multi threaded TCG.
When reset happens, all the CPUs are in halted state. First CPU is brought out
of reset and secondary CPUs would be initialized by the guest kernel using a
rtas call start-cpu.
However, in case of TCG, decrementer interrupts keep on coming and waking the
secondary CPUs up.
These secondary CPUs would see the decrementer interrupt pending, which makes
cpu::has_work() to bring them out of wait loop and start executing
tcg_exec_cpu().
The problem with this is all the CPUs wake up and start booting SLOF image,
causing the following exception(4 CPUs TCG VM):
[ 81.440850] reboot: Restarting system
SLOF
S
SLOF
SLOFLOF[0[0m
**
QEMU Starting
Build Date = Mar 3 2017 13:29:19
FW Version = git-66d250ef0fd06bb8
[0m **
QEMU Starting
Build Date = Mar 3 2017 13:29:19
FW Version = git-66d250ef0fd06bb8
[0m *m**[?25l
**
QEMU Starting
Build Date = Mar 3 2017 13:29:19
FW Version = git-66d250ef0fd06bb8
***
QEMU Starting
Build Date = Mar 3 2017 13:29:19
FW Version = git-66d250ef0fd06bb8
ERROR: Flatten device tree not available!
exception 300
SRR0 = 60e4 SRR1 = 80008000
SPRG2 = 0040 SPRG3 = 4bd8
ERROR: Flatten device tree not available!
exception 300
SRR0 = 60e4 SRR1 = 80008000
SPRG2 = 0040 SPRG3 = 4bd8
Reported-by: Bharata B Rao
Signed-off-by: Nikunj A Dadhania
---
Note: Similar changes would be required for powernv as well.
Haven't got time to test it there.
---
hw/ppc/spapr.c | 1 +
hw/ppc/spapr_cpu_core.c | 1 +
hw/ppc/spapr_rtas.c | 1 +
target/ppc/cpu.h| 7 +++
target/ppc/translate_init.c | 9 +
5 files changed, 19 insertions(+)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 01dda9e..fba2ef5 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1370,6 +1370,7 @@ static void ppc_spapr_reset(void)
first_ppc_cpu->env.gpr[3] = fdt_addr;
first_ppc_cpu->env.gpr[5] = 0;
first_cpu->halted = 0;
+first_ppc_cpu->env.in_reset = 0;
first_ppc_cpu->env.nip = SPAPR_ENTRY_POINT;
spapr->cas_reboot = false;
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 029a141..c100213 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -33,6 +33,7 @@ static void spapr_cpu_reset(void *opaque)
* reset code and the rest are explicitly started up by the guest
* using an RTAS call */
cs->halted = 1;
+env->in_reset = 1;
env->spr[SPR_HIOR] = 0;
diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
index 94a2799..eaf0afb 100644
--- a/hw/ppc/spapr_rtas.c
+++ b/hw/ppc/spapr_rtas.c
@@ -177,6 +177,7 @@ static void rtas_start_cpu(PowerPCCPU *cpu_,
sPAPRMachineState *spapr,
env->nip = start;
env->gpr[3] = r3;
cs->halted = 0;
+env->in_reset = 0;
spapr_cpu_set_endianness(cpu);
spapr_cpu_update_tb_offset(cpu);
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index d10808d..eb88bcb 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1013,6 +1013,13 @@ struct CPUPPCState {
int access_type; /* when a memory exception occurs, the access
type is stored here */
+/* CPU in reset, shouldn't process any interrupts.
+ *
+ * Decrementer interrupts in TCG can still wake the CPU up. Make sure that
+ * when this variable is set, cpu_has_work_* should return false.
+ */
+int in_reset;
+
CPU_COMMON
/* MMU context - only relevant for full system emulation */
diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
index 56a0ab2..64f4348 100644
--- a/target/ppc/translate_init.c
+++ b/target/ppc/translate_init.c
@@ -8561,6 +8561,9 @@ static bool cpu_has_work_POWER7(CPUState *cs)
CPUPPCState *env = &cpu->env;
if (cs->halted) {
+if (env->in_reset) {
+return false;
+}
if (!(cs->interrupt_request & CPU_INTERRUPT_HARD)) {
return false;
}
@@ -8718,6 +8721,9 @@ static bool cpu_has_work_POWER8(CPUState *cs)
CPUPPCState *env = &cpu->env;
if (cs->halted) {
+if (env->in_reset) {
+return false;
+}
if (!(cs->interrupt_request & CPU_INTERRUPT_HARD)) {
return false;
}
@@ -8899,6 +8905,9 @@ static bool cpu_has_work_POWER9(CPUState *cs)
CPUPPCState *env = &cpu->env;
if (cs->halted) {
+if (env->in_reset) {
+return false;
+}
if (!(cs->interrupt_request & CPU_INTERRUPT_HARD)) {
return false;
}
--
2.9.3
Re: [Qemu-devel] [PATCH v5 1/2] spapr: Add a "no HPT" encoding to HTAB migration stream
On Thu, Jun 08, 2017 at 02:28:48PM +1000, David Gibson wrote: > On Wed, Jun 07, 2017 at 01:24:52PM +0530, Bharata B Rao wrote: > > Add a "no HPT" encoding (using value -1) to the HTAB migration > > stream (in the place of HPT size) when the guest doesn't allocate HPT. > > This will help the target side to match target HPT with the source HPT > > and thus enable successful migration. > > > > Suggested-by: David Gibson > > Signed-off-by: Bharata B Rao > > As dicussed in the thread on the previous version, I still think you > can use section_hdr == 0 as the no-HPT encoding (matching htab_shift > == 0. Bharata discussed this with me on IRC and pointed out I was mistaken. It thought the overall stream header was distinct from the incremental content, but it's not. The zero value in the header marks this "chunk" of data as being continuing incremental content (the "middle" of the stream) rather than the initial stream header which gives the HPT size. So, yes, we do need a new and different encoding for "no HPT". -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
Re: [Qemu-devel] [PATCH] util/qemu-sockets: Drop unused helper socket_address_to_string()
Mao Zhongyi writes: > Signed-off-by: Mao Zhongyi socket_address_to_string() was added in commit 7e84495 "Change net/socket.c to use socket_*() functions". The commit was reverted (commit 6160183), but the revert left in the helper. It was then redone (commit 883e4f7), and reverted again (commit 6701e55). No other users have emerged. If they ever do, the helper is one git-revert away. Reviewed-by: Markus Armbruster
Re: [Qemu-devel] [PATCH 2/3] exec: simplify address_space_get_iotlb_entry
On Wed, Jun 07, 2017 at 04:07:20PM +0300, Michael S. Tsirkin wrote: > On Wed, Jun 07, 2017 at 11:44:43AM +0800, Peter Xu wrote: > > On Wed, Jun 07, 2017 at 09:47:05AM +1000, David Gibson wrote: > > > On Tue, Jun 06, 2017 at 04:34:30PM +0200, Paolo Bonzini wrote: > > > > > > > > > > > > On 05/06/2017 05:07, Peter Xu wrote: > > > > > I don't sure whether it'll be a good interface for IOTLB. AFAIU at > > > > > least for VT-d, the IOMMU translation is page aligned which is defined > > > > > by spec, so it makes sense that (again at least for VT-d) here we'd > > > > > better just use page_mask/addr_mask. > > > > > > > > > > That's also how I know about IOMMU in general - I assume it do the > > > > > translations always with page masks (never arbitary length), though > > > > > page size can differ from platfrom to platform, that's why here the > > > > > IOTLB interface used addr_mask, then it works for all platforms. I > > > > > don't know whether I'm 100% correct here though. > > > > > > > > > > Maybe David/Paolo/... would comment as well? > > > > > > > > I would ask David. There are PowerPC MMUs that allow fast lookup of > > > > arbitrarily-sized windows (not necessarily power of two), > > > > > > Uh.. I'm not sure what you mean here. You might be thinking of the > > > BATs which really old (32-bit) PowerPC MMUs had - those allow > > > arbitrary large block translations, but they do have to be a power of > > > two. > > > > > > > so maybe the > > > > IOMMUs can do the same. > > > > > > The only Power IOMMU I know about uses a fixed, power-of-two page size > > > per DMA window. > > > > If so, I would still be inclined to keep using masks for QEMU IOTLB. > > Then, my first two patches should still stand. > > > > I am just afraid that not using masks will diverge the emulation from > > real hardware and brings trouble one day. > > > > For vhost IOTLB interface, it does not need to be strictly aligned to > > QEMU IOMMU IOTLB definition, and that's how it's working now (current > > vhost iotlb allows arbitary length, and I think it's good). So imho we > > don't really need to worry about the performance - after all, we can > > do everything customized for vhost, just like what patch 3 did (yeah, > > it can be better...). > > > > Thanks, > > Pre-faults is also something that does not happen on real hardware. > And it's about security so a bigger issue. > > If I had to choose between that and using non-power-of-2 in > the API, I'd go for non-power-of-2. Let backends that can only > support power of 2 split it up to multiple transactions. The problem is that when I was fixing the problem that vhost had with PT (a764040, "exec: abstract address_space_do_translate()"), I did broke the IOTLB translation a bit (it was using page masks). IMHO we need to fix it first for correctness (patch 1/2). For patch 3, if we can have Jason's patch to allow dynamic iommu_platform switching, that'll be the best, then I can rewrite patch 3 with the switching logic rather than caching anything. But IMHO that can be separated from patch 1/2 if you like. Or do you have better suggestion on how should we fix it? Thanks, -- Peter Xu
Re: [Qemu-devel] [Qemu-ppc] [PATCHv5 0/4] Clean up compatibility mode handling
On Thu, 2017-06-08 at 14:17 +1000, David Gibson wrote: > On Fri, Jun 02, 2017 at 01:15:03PM +1000, David Gibson wrote: > > This is a rebased and revised version of my patches revising CPU > > compatiblity mode handling on ppc, last posted in November. Since > > then, many of the patches have already been merged (some for 2.9, > > some > > since). This is what's left. > > > > * There was conceptual confusion about what a compatibility mode > > means, and how it interacts with the machine type. This cleans > > that up, clarifying that a compatibility mode (as an externally > > set > > option) only makes sense on machine types that don't permit the > > guest hypervisor privilege (i.e. 'pseries') > > > > * It was previously the user's (or management layer's) > > responsibility > > to determine compatibility of CPUs on either end for migration. > > This uses the compatibility modes to check that properly during > > an > > incoming migration. > > Anyone willing to give some review and/or testing, particularly for > patch 2/4. > Sorry I had been playing around with this and meant to give it a Tested-by
Re: [Qemu-devel] [PATCH v1 1/1] char-socket: Don't report TCP socket waiting as an error
Alistair Francis writes: > On Wed, Jun 7, 2017 at 12:19 AM, Markus Armbruster wrote: >> Paolo Bonzini writes: >> >>> On 06/06/2017 18:30, Alistair Francis wrote: > > This is somehow confusing. I don't think it is worth having another > qemu_log_stderr() function rather than using error_report() but this very > call might deserve a comment explaining this unusual use. What do you > think? The problem with stderr is that this isn't an error. Some uses of QEMU (inside Eclipse for example) flag everything printed on stderr as red which confuses users that they are seeing an error when they really aren't. >>> >>> But they are wrong. >> >> Concur. We also print warnings and informational messages to stderr. >> >> We should make errors easy to recognize. Fortunately, error_report() >> prints errors to stderr in a rigid format. Unfortunately, error >> messages bypassing error_report() still exist in places. We suck. >> >> The format is >> >> timestamp-if-enabled progname ':' location message >> >> timestamp-if-enabled is normally empty. With -msg timestamp=on, it's >> the current time in ISO 8601 format, followed by a space. >> >> progname is the program name (main()'s argv[0]). >> >> location is either empty, or a reference to the command line or a >> configuration file. >> >> See error_vreport() for details. > > Ok, but this isn't an error, it's more information. So it sounds like > we should still print to stderr but not print in the format described > above? Yes. I explained the error message format to show how to distinguish actual errors from other stuff.
Re: [Qemu-devel] [Qemu-ppc] [PATCHv5 2/4] pseries: Move CPU compatibility property to machine
On Fri, 2017-06-02 at 13:15 +1000, David Gibson wrote:
> Server class POWER CPUs have a "compat" property, which is used to
> set the
> backwards compatibility mode for the processor. However, this only
> makes
> sense for machine types which don't give the guest access to
> hypervisor
> privilege - otherwise the compatibility level is under the guest's
> control.
>
> To reflect this, this removes the CPU 'compat' property and instead
> creates a 'max-cpu-compat' property on the pseries machine. Strictly
> speaking this breaks compatibility, but AFAIK the 'compat' option was
> never (directly) used with -device or device_add.
>
> The option was used with -cpu. So, to maintain compatibility, this
> patch adds a hack to the cpu option parsing to strip out any compat
> options supplied with -cpu and set them on the machine property
> instead of the now deprecated cpu property.
>
> Signed-off-by: David Gibson
Looks good to me and no longer segfaults :)
Tried a few configurations and behaves as expected:
Tested-by: Suraj Jitindar Singh
> ---
> hw/ppc/spapr.c | 6 ++-
> hw/ppc/spapr_cpu_core.c | 55 +++-
> hw/ppc/spapr_hcall.c| 8 ++--
> include/hw/ppc/spapr.h | 12 --
> target/ppc/compat.c | 102
>
> target/ppc/cpu.h| 5 ++-
> target/ppc/translate_init.c | 86 +++---
> ---
> 7 files changed, 201 insertions(+), 73 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index ab3aab1..3c4e88f 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2134,7 +2134,7 @@ static void ppc_spapr_init(MachineState
> *machine)
> machine->cpu_model = kvm_enabled() ? "host" : smc-
> >tcg_default_cpu;
> }
>
> -ppc_cpu_parse_features(machine->cpu_model);
> +spapr_cpu_parse_features(spapr);
>
> spapr_init_cpus(spapr);
>
> @@ -2497,6 +2497,10 @@ static void spapr_machine_initfn(Object *obj)
> " place of standard EPOW events
> when possible"
> " (required for memory hot-
> unplug support)",
> NULL);
> +
> +ppc_compat_add_property(obj, "max-cpu-compat", &spapr-
> >max_compat_pvr,
> +"Maximum permitted CPU compatibility
> mode",
> +&error_fatal);
> }
>
> static void spapr_machine_finalizefn(Object *obj)
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index ff7058e..846d9e7 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -20,6 +20,57 @@
> #include "sysemu/numa.h"
> #include "qemu/error-report.h"
>
> +void spapr_cpu_parse_features(sPAPRMachineState *spapr)
> +{
> +/*
> + * Backwards compatibility hack:
> + *
> + * CPUs had a "compat=" property which didn't make sense for
> + * anything except pseries. It was replaced by "max-cpu-
> compat"
> + * machine option. This supports old command lines like
> + * -cpu POWER8,compat=power7
> + * By stripping the compat option and applying it to the
> machine
> + * before passing it on to the cpu level parser.
> + */
> +gchar **inpieces;
> +int i, j;
> +gchar *compat_str = NULL;
> +
> +inpieces = g_strsplit(MACHINE(spapr)->cpu_model, ",", 0);
> +
> +/* inpieces[0] is the actual model string */
> +i = 1;
> +j = 1;
> +while (inpieces[i]) {
> +if (g_str_has_prefix(inpieces[i], "compat=")) {
> +/* in case of multiple compat= options */
> +g_free(compat_str);
> +compat_str = inpieces[i];
> +} else {
> +j++;
> +}
> +
> +i++;
> +/* Excise compat options from list */
> +inpieces[j] = inpieces[i];
> +}
> +
> +if (compat_str) {
> +char *val = compat_str + strlen("compat=");
> +gchar *newprops = g_strjoinv(",", inpieces);
> +
> +object_property_set_str(OBJECT(spapr), val, "max-cpu-
> compat",
> +&error_fatal);
> +
> +ppc_cpu_parse_features(newprops);
> +g_free(newprops);
> +} else {
> +ppc_cpu_parse_features(MACHINE(spapr)->cpu_model);
> +}
> +
> +g_strfreev(inpieces);
> +}
> +
> static void spapr_cpu_reset(void *opaque)
> {
> sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> @@ -70,10 +121,10 @@ static void spapr_cpu_init(sPAPRMachineState
> *spapr, PowerPCCPU *cpu,
> /* Enable PAPR mode in TCG or KVM */
> cpu_ppc_set_papr(cpu, PPC_VIRTUAL_HYPERVISOR(spapr));
>
> -if (cpu->max_compat) {
> +if (spapr->max_compat_pvr) {
> Error *local_err = NULL;
>
> -ppc_set_compat(cpu, cpu->max_compat, &local_err);
> +ppc_set_compat(cpu, spapr->max_compat_pvr, &local_err);
> if (local_err) {
> error_propagate(errp, local_e
Re: [Qemu-devel] [PATCH v3 3/5] xics: setup cpu at realize time
On 06/07/2017 10:55 PM, Greg Kurz wrote:
> On Wed, 7 Jun 2017 20:11:38 +0200
> Cédric Le Goater wrote:
>
>> On 06/07/2017 07:17 PM, Greg Kurz wrote:
>>> Until recently, spapr used to allocate ICPState objects for the lifetime
>>> of the machine. They would only be associated to vCPUs in xics_cpu_setup()
>>> when plugging a CPU core.
>>>
>>> Now that ICPState objects have the same lifecycle as vCPUs, it is
>>> possible to associate them during realization.
>>>
>>> This patch hence open-codes xics_cpu_setup() in icp_realize(). The vCPU
>>> is passed as a property. Note that vCPU now needs to be realized first
>>> for the IRQs to be allocated. It also needs to resetted before ICPState
>>> realization in order to synchronize with KVM.
>>>
>>> Since ICPState objects are freed when unrealized, xics_cpu_destroy() isn't
>>> needed anymore and can be safely dropped.
>>
>> I like the idea but I think the assignment of ->cs attribute should be
>> moved under icp_kvm_cpu_setup(), as it is only used under xics_kvm by
>> the kvm_vcpu_ioctl() calls.
>>
>
> Well, cs->cpu_index is also used for traces and to implement the 'info pic'
> monitor command.
yes. But, these are just printfs.
>> Ideally, we should also introduce :
>>
>> struct KvmState {
>> ICPState parent_obj;
>>
>> CPUState *cs;
>> };
>>
>> That would be cleaner, but that might introduce some migration compat
>> issues again ...
>>
>
> No, as long as it doesn't change state, we're good. My concern is more
> about how to pass cs to xics_kvm
That can be done in the cpu_setup handler.
> and the cs->cpu_index to xics.
The printfs are interesting to have but not vital.
David has a good point for keeping ->cs. So let's abandon the idea.
>> Some minor comments below,
>>
>> Thanks,
>>
>> C.
>>
>>> Signed-off-by: Greg Kurz
>>> ---
>>> hw/intc/xics.c | 76
>>> ---
>>> hw/ppc/pnv_core.c | 16 --
>>> hw/ppc/spapr_cpu_core.c | 21 ++---
>>> include/hw/ppc/xics.h |2 -
>>> 4 files changed, 48 insertions(+), 67 deletions(-)
>>>
>>> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
>>> index ec73f02144c9..330441ff7fe8 100644
>>> --- a/hw/intc/xics.c
>>> +++ b/hw/intc/xics.c
>>> @@ -38,50 +38,6 @@
>>> #include "monitor/monitor.h"
>>> #include "hw/intc/intc.h"
>>>
>>> -void xics_cpu_destroy(XICSFabric *xi, PowerPCCPU *cpu)
>>> -{
>>> -CPUState *cs = CPU(cpu);
>>> -ICPState *icp = ICP(cpu->intc);
>>> -
>>> -assert(icp);
>>> -assert(cs == icp->cs);
>>> -
>>> -icp->output = NULL;
>>> -icp->cs = NULL;
>>> -}
>>> -
>>> -void xics_cpu_setup(XICSFabric *xi, PowerPCCPU *cpu, ICPState *icp)
>>> -{
>>> -CPUState *cs = CPU(cpu);
>>> -CPUPPCState *env = &cpu->env;
>>> -ICPStateClass *icpc;
>>> -
>>> -assert(icp);
>>> -
>>> -cpu->intc = OBJECT(icp);
>>> -icp->cs = cs;
>>> -
>>> -icpc = ICP_GET_CLASS(icp);
>>> -if (icpc->cpu_setup) {
>>> -icpc->cpu_setup(icp, cpu);
>>> -}
>>> -
>>> -switch (PPC_INPUT(env)) {
>>> -case PPC_FLAGS_INPUT_POWER7:
>>> -icp->output = env->irq_inputs[POWER7_INPUT_INT];
>>> -break;
>>> -
>>> -case PPC_FLAGS_INPUT_970:
>>> -icp->output = env->irq_inputs[PPC970_INPUT_INT];
>>> -break;
>>> -
>>> -default:
>>> -error_report("XICS interrupt controller does not support this CPU "
>>> - "bus model");
>>> -abort();
>>> -}
>>> -}
>>> -
>>> void icp_pic_print_info(ICPState *icp, Monitor *mon)
>>> {
>>> int cpu_index = icp->cs ? icp->cs->cpu_index : -1;
>>> @@ -343,6 +299,8 @@ static void icp_realize(DeviceState *dev, Error **errp)
>>> {
>>> ICPState *icp = ICP(dev);
>>> ICPStateClass *icpc = ICP_GET_CLASS(dev);
>>> +PowerPCCPU *cpu;
>>> +CPUPPCState *env;
>>> Object *obj;
>>> Error *err = NULL;
>>>
>>> @@ -355,6 +313,36 @@ static void icp_realize(DeviceState *dev, Error **errp)
>>>
>>> icp->xics = XICS_FABRIC(obj);
>>>
>>> +obj = object_property_get_link(OBJECT(dev), "cs", &err);
>>> +if (!obj) {
>>> +error_setg(errp, "%s: required link 'xics' not found: %s",
>>> + __func__, error_get_pretty(err));
>>> +return;
>>> +}
>>> +
>>> +cpu = POWERPC_CPU(obj);
>>> +cpu->intc = OBJECT(icp);
>>> +icp->cs = CPU(obj);
>>
>> only xics_kvm needs ->cs.
>>
>
> yeah, maybe the base class should only have a cpu_index field:
>
> icp->cpu_index = CPU(obj)->cpu_index;
arg, no. please, let's not add another cpu index :)
>>> +
>>> +if (icpc->cpu_setup) {
>>> +icpc->cpu_setup(icp, cpu);
>>> +}
>>> +
>>> +env = &cpu->env;
>>> +switch (PPC_INPUT(env)) {
>>> +case PPC_FLAGS_INPUT_POWER7:
>>> +icp->output = env->irq_inputs[POWER7_INPUT_INT];
>>> +break;
>>> +
>>> +case PPC_FLAGS_INPUT_970:
>>> +icp->output = env->irq_inputs[PPC970_INPUT_INT]
[Qemu-devel] [PATCH V2] monitor: Add -a (all) option to info registers
The info registers command in the qemu monitor is used to dump register
values.
Currently this command uses the monitor cpu (which can be set by the
user) as the cpu for whose registers will be dumped. Sometimes it is
useful to see the registers for all cpus and currently this requires
setting the monitor cpu and the re-running the command for each cpu
in the system. I would be nice if there was an easier way to do this.
Add the "-a" option to the info registers command to dump the register
values for all cpus.
Signed-off-by: Suraj Jitindar Singh
---
Change Log:
V1 -> V2:
- Add CPU number to beginning of register dump
---
hmp-commands-info.hx | 6 +++---
monitor.c| 21 -
2 files changed, 19 insertions(+), 8 deletions(-)
diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index ae16901..ba98e58 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -100,9 +100,9 @@ ETEXI
{
.name = "registers",
-.args_type = "",
-.params = "",
-.help = "show the cpu registers",
+.args_type = "cpustate_all:-a",
+.params = "[-a]",
+.help = "show the cpu registers (-a: all - show register info
for all cpus)",
.cmd= hmp_info_registers,
},
diff --git a/monitor.c b/monitor.c
index baa73c9..1629ad1 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1078,13 +1078,24 @@ int monitor_get_cpu_index(void)
static void hmp_info_registers(Monitor *mon, const QDict *qdict)
{
-CPUState *cs = mon_get_cpu();
+bool all_cpus = qdict_get_try_bool(qdict, "cpustate_all", false);
+CPUState *cs;
-if (!cs) {
-monitor_printf(mon, "No CPU available\n");
-return;
+if (all_cpus) {
+CPU_FOREACH(cs) {
+monitor_printf(mon, "\nCPU#%d\n", cs->cpu_index);
+cpu_dump_state(cs, (FILE *)mon, monitor_fprintf, CPU_DUMP_FPU);
+}
+} else {
+cs = mon_get_cpu();
+
+if (!cs) {
+monitor_printf(mon, "No CPU available\n");
+return;
+}
+
+cpu_dump_state(cs, (FILE *)mon, monitor_fprintf, CPU_DUMP_FPU);
}
-cpu_dump_state(cs, (FILE *)mon, monitor_fprintf, CPU_DUMP_FPU);
}
static void hmp_info_jit(Monitor *mon, const QDict *qdict)
--
2.9.4
Re: [Qemu-devel] [PATCH] monitor: Add -a (all) option to info registers
On Wed, 2017-06-07 at 20:16 +0100, Dr. David Alan Gilbert wrote:
> * Suraj Jitindar Singh (sjitindarsi...@gmail.com) wrote:
> > The info registers command in the qemu monitor is used to dump
> > register
> > values.
> >
> > Currently this command uses the monitor cpu (which can be set by
> > the
> > user) as the cpu for whose registers will be dumped. Sometimes it
> > is
> > useful to see the registers for all cpus and currently this
> > requires
> > setting the monitor cpu and the re-running the command for each cpu
> > in the system. I would be nice if there was an easier way to do
> > this.
> >
> > Add the "-a" option to the info registers command to dump the
> > register
> > values for all cpus.
> >
> > Signed-off-by: Suraj Jitindar Singh
>
> Hi Suraj,
> That looks pretty good, one minor suggestion below
Thanks, V2 to come :)
>
> > ---
> > hmp-commands-info.hx | 6 +++---
> > monitor.c| 20 +++-
> > 2 files changed, 18 insertions(+), 8 deletions(-)
> >
> > diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> > index ae16901..ba98e58 100644
> > --- a/hmp-commands-info.hx
> > +++ b/hmp-commands-info.hx
> > @@ -100,9 +100,9 @@ ETEXI
> >
> > {
> > .name = "registers",
> > -.args_type = "",
> > -.params = "",
> > -.help = "show the cpu registers",
> > +.args_type = "cpustate_all:-a",
> > +.params = "[-a]",
> > +.help = "show the cpu registers (-a: all - show
> > register info for all cpus)",
> > .cmd= hmp_info_registers,
> > },
> >
> > diff --git a/monitor.c b/monitor.c
> > index baa73c9..5875f88 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -1078,13 +1078,23 @@ int monitor_get_cpu_index(void)
> >
> > static void hmp_info_registers(Monitor *mon, const QDict *qdict)
> > {
> > -CPUState *cs = mon_get_cpu();
> > +bool all_cpus = qdict_get_try_bool(qdict, "cpustate_all",
> > false);
> > +CPUState *cs;
> >
> > -if (!cs) {
> > -monitor_printf(mon, "No CPU available\n");
> > -return;
> > +if (all_cpus) {
> > +CPU_FOREACH(cs) {
>
> Could we add a monitor_printf here giving the CPU number ?
> It would just make it a little easier when you have big list of CPUs
> and each has lots of registers to be able to see which one you're
> looking at.
Good idea, I never noticed because the CPU# is in the powerpc cpu dump
anyway.
I'll add it and send a V2.
>
>
> Thanks,
>
> Dave
>
> > +cpu_dump_state(cs, (FILE *)mon, monitor_fprintf,
> > CPU_DUMP_FPU);
> > +}
> > +} else {
> > +cs = mon_get_cpu();
> > +
> > +if (!cs) {
> > +monitor_printf(mon, "No CPU available\n");
> > +return;
> > +}
> > +
> > +cpu_dump_state(cs, (FILE *)mon, monitor_fprintf,
> > CPU_DUMP_FPU);
> > }
> > -cpu_dump_state(cs, (FILE *)mon, monitor_fprintf,
> > CPU_DUMP_FPU);
> > }
> >
> > static void hmp_info_jit(Monitor *mon, const QDict *qdict)
> > --
> > 2.9.4
>
> Dave
>
> >
>
> --
> Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [Qemu-devel] [PATCH v2 1/1] qemu/migration: fix the double free problem on from_src_file
在 2017/6/7 20:18, Dr. David Alan Gilbert 写道: * QingFeng Hao (ha...@linux.vnet.ibm.com) wrote: 在 2017/6/6 20:49, Kevin Wolf 写道: Am 06.06.2017 um 07:24 hat QingFeng Hao geschrieben: I can't tell for postcopy_ram_listen_thread() - commit 660819b didn't seem to remove a qemu_fclose() call there, but I can't see one left behind either. Was the file leaked before commit 660819b or am I missing something? I don't think so because loadvm_postcopy_handle_listen creates thread postcopy_ram_listen_thread and passes mis->from_src_file as its arg, which will be closed by migration_incoming_state_destroy. What confuses me is in the series function calls of qemu_loadvm_state_main etc, argument f looks to be redundant as mis already contains from_src_file which equals to f. In postcopy qemu_loadvm_state_main is called with two different file arguments but the same mis argument; see loadvm_handle_cmd_packaged for the other case where it's called on a packaged-file blob. yes, you are right, I missed that one. :) Furthermore, mis may be also redundant as it can be got via migration_incoming_get_current. Thanks! We keep changing our minds about the preferred style. Sometimes we think it's best to pass the pointer, sometimes we think it's best to call get_current. Got it. Thanks! Dave Kevin -- Regards QingFeng Hao -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK -- Regards QingFeng Hao
[Qemu-devel] [PATCH] vl: Fix broken thread=xxx option of the --accel parameter
Commit bde4d9205 ("Fix the -accel parameter and the documentation for
'hax'") introduced a regression by adding a new local accel_opts
variable which shadows the variable with the same name that is
declared at the beginning of the main() scope. This causes the
qemu_tcg_configure() call later to be always called with NULL, so
that the thread=xxx option gets ignored. Fix it by removing the
local accel_opts variable and use "opts" instead, which is meant
for storing temporary QemuOpts values.
And while we're at it, also change the exit(1) here to exit(0)
since asking for help is not an error.
Fixes: bde4d9205ee9def98852ff6054cdef4efd74e1f8
Reported-by: Markus Armbruster
Reported-by: Emilio G. Cota
Signed-off-by: Thomas Huth
---
vl.c | 13 +
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/vl.c b/vl.c
index be4dcf2..5aba544 100644
--- a/vl.c
+++ b/vl.c
@@ -3757,21 +3757,18 @@ int main(int argc, char **argv, char **envp)
qdev_prop_register_global(&kvm_pit_lost_tick_policy);
break;
}
-case QEMU_OPTION_accel: {
-QemuOpts *accel_opts;
-
+case QEMU_OPTION_accel:
accel_opts = qemu_opts_parse_noisily(qemu_find_opts("accel"),
optarg, true);
optarg = qemu_opt_get(accel_opts, "accel");
if (!optarg || is_help_option(optarg)) {
error_printf("Possible accelerators: kvm, xen, hax,
tcg\n");
-exit(1);
+exit(0);
}
-accel_opts = qemu_opts_create(qemu_find_opts("machine"), NULL,
- false, &error_abort);
-qemu_opt_set(accel_opts, "accel", optarg, &error_abort);
+opts = qemu_opts_create(qemu_find_opts("machine"), NULL,
+false, &error_abort);
+qemu_opt_set(opts, "accel", optarg, &error_abort);
break;
-}
case QEMU_OPTION_usb:
olist = qemu_find_opts("machine");
qemu_opts_parse_noisily(olist, "usb=on", false);
--
1.8.3.1
[Qemu-devel] [PATCH 6/6] spapr: Clean up DRC set_isolation_state() path
There are substantial differences in the various paths through
set_isolation_state(), both for setting to ISOLATED versus UNISOLATED
state and for logical versus physical DRCs.
So, split the set_isolation_state() method into isolate() and unisolate()
methods, and give it different implementations for the two DRC types.
Factor some minimal common checks, including for valid indicator values
(which we weren't previously checking) into rtas_set_isolation_state().
Signed-off-by: David Gibson
---
hw/ppc/spapr_drc.c | 145 -
include/hw/ppc/spapr_drc.h | 6 +-
2 files changed, 105 insertions(+), 46 deletions(-)
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 9e01d7b..90c0fde 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -46,30 +46,64 @@ uint32_t spapr_drc_index(sPAPRDRConnector *drc)
| (drc->id & DRC_INDEX_ID_MASK);
}
-static uint32_t set_isolation_state(sPAPRDRConnector *drc,
-sPAPRDRIsolationState state)
+static uint32_t drc_isolate_physical(sPAPRDRConnector *drc)
{
-trace_spapr_drc_set_isolation_state(spapr_drc_index(drc), state);
-
/* if the guest is configuring a device attached to this DRC, we
* should reset the configuration state at this point since it may
* no longer be reliable (guest released device and needs to start
* over, or unplug occurred so the FDT is no longer valid)
*/
-if (state == SPAPR_DR_ISOLATION_STATE_ISOLATED) {
-g_free(drc->ccs);
-drc->ccs = NULL;
-}
+g_free(drc->ccs);
+drc->ccs = NULL;
-if (state == SPAPR_DR_ISOLATION_STATE_UNISOLATED) {
-/* cannot unisolate a non-existent resource, and, or resources
- * which are in an 'UNUSABLE' allocation state. (PAPR 2.7, 13.5.3.5)
- */
-if (!drc->dev ||
-drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE) {
-return RTAS_OUT_NO_SUCH_INDICATOR;
+drc->isolation_state = SPAPR_DR_ISOLATION_STATE_ISOLATED;
+
+/* if we're awaiting release, but still in an unconfigured state,
+ * it's likely the guest is still in the process of configuring
+ * the device and is transitioning the devices to an ISOLATED
+ * state as a part of that process. so we only complete the
+ * removal when this transition happens for a device in a
+ * configured state, as suggested by the state diagram from PAPR+
+ * 2.7, 13.4
+ */
+if (drc->awaiting_release) {
+uint32_t drc_index = spapr_drc_index(drc);
+if (drc->configured) {
+trace_spapr_drc_set_isolation_state_finalizing(drc_index);
+spapr_drc_detach(drc, DEVICE(drc->dev), NULL);
+} else {
+trace_spapr_drc_set_isolation_state_deferring(drc_index);
}
}
+drc->configured = false;
+
+return RTAS_OUT_SUCCESS;
+}
+
+static uint32_t drc_unisolate_physical(sPAPRDRConnector *drc)
+{
+/* cannot unisolate a non-existent resource, and, or resources
+ * which are in an 'UNUSABLE' allocation state. (PAPR 2.7,
+ * 13.5.3.5)
+ */
+if (!drc->dev) {
+return RTAS_OUT_NO_SUCH_INDICATOR;
+}
+
+drc->isolation_state = SPAPR_DR_ISOLATION_STATE_UNISOLATED;
+
+return RTAS_OUT_SUCCESS;
+}
+
+static uint32_t drc_isolate_logical(sPAPRDRConnector *drc)
+{
+/* if the guest is configuring a device attached to this DRC, we
+ * should reset the configuration state at this point since it may
+ * no longer be reliable (guest released device and needs to start
+ * over, or unplug occurred so the FDT is no longer valid)
+ */
+g_free(drc->ccs);
+drc->ccs = NULL;
/*
* Fail any requests to ISOLATE the LMB DRC if this LMB doesn't
@@ -81,35 +115,47 @@ static uint32_t set_isolation_state(sPAPRDRConnector *drc,
* If the LMB being removed doesn't belong to a DIMM device that is
* actually being unplugged, fail the isolation request here.
*/
-if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_LMB) {
-if ((state == SPAPR_DR_ISOLATION_STATE_ISOLATED) &&
- !drc->awaiting_release) {
-return RTAS_OUT_HW_ERROR;
-}
+if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_LMB
+&& !drc->awaiting_release) {
+return RTAS_OUT_HW_ERROR;
}
-drc->isolation_state = state;
+drc->isolation_state = SPAPR_DR_ISOLATION_STATE_ISOLATED;
-if (drc->isolation_state == SPAPR_DR_ISOLATION_STATE_ISOLATED) {
-/* if we're awaiting release, but still in an unconfigured state,
- * it's likely the guest is still in the process of configuring
- * the device and is transitioning the devices to an ISOLATED
- * state as a part of that process. so we only complete the
- * removal when this transition happens for a device in a
- * configured state, as suggested by the state diagram from
- * PAPR+ 2.
[Qemu-devel] [PATCH 5/6] spapr: Clean up DRC set_allocation_state path
The allocation-state indicator should only actually be implemented for
"logical" DRCs, not physical ones. Factor a check for this, and also for
valid indicator state values into rtas_set_allocation_state(). Because
they don't exist for physical DRCs, there's no reason that we'd ever want
more than one method implementation, so it can just be a plain function.
In addition, the setting to USABLE and setting to UNUSABLE paths in
set_allocation_state() don't actually have much in common. So, split the
method separate functions for each parameter value (drc_set_usable()
and drc_set_unusable()).
Signed-off-by: David Gibson
---
hw/ppc/spapr_drc.c | 85 +-
include/hw/ppc/spapr_drc.h | 2 --
2 files changed, 46 insertions(+), 41 deletions(-)
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 7e17f34..9e01d7b 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -114,44 +114,43 @@ static uint32_t set_isolation_state(sPAPRDRConnector *drc,
return RTAS_OUT_SUCCESS;
}
-static uint32_t set_allocation_state(sPAPRDRConnector *drc,
- sPAPRDRAllocationState state)
+static uint32_t drc_set_usable(sPAPRDRConnector *drc)
{
-trace_spapr_drc_set_allocation_state(spapr_drc_index(drc), state);
-
-if (state == SPAPR_DR_ALLOCATION_STATE_USABLE) {
-/* if there's no resource/device associated with the DRC, there's
- * no way for us to put it in an allocation state consistent with
- * being 'USABLE'. PAPR 2.7, 13.5.3.4 documents that this should
- * result in an RTAS return code of -3 / "no such indicator"
+/* if there's no resource/device associated with the DRC, there's
+ * no way for us to put it in an allocation state consistent with
+ * being 'USABLE'. PAPR 2.7, 13.5.3.4 documents that this should
+ * result in an RTAS return code of -3 / "no such indicator"
+ */
+if (!drc->dev) {
+return RTAS_OUT_NO_SUCH_INDICATOR;
+}
+if (drc->awaiting_release && drc->awaiting_allocation) {
+/* kernel is acknowledging a previous hotplug event
+ * while we are already removing it.
+ * it's safe to ignore awaiting_allocation here since we know the
+ * situation is predicated on the guest either already having done
+ * so (boot-time hotplug), or never being able to acquire in the
+ * first place (hotplug followed by immediate unplug).
*/
-if (!drc->dev) {
-return RTAS_OUT_NO_SUCH_INDICATOR;
-}
-if (drc->awaiting_release && drc->awaiting_allocation) {
-/* kernel is acknowledging a previous hotplug event
- * while we are already removing it.
- * it's safe to ignore awaiting_allocation here since we know the
- * situation is predicated on the guest either already having done
- * so (boot-time hotplug), or never being able to acquire in the
- * first place (hotplug followed by immediate unplug).
- */
-drc->awaiting_allocation_skippable = true;
-return RTAS_OUT_NO_SUCH_INDICATOR;
-}
+drc->awaiting_allocation_skippable = true;
+return RTAS_OUT_NO_SUCH_INDICATOR;
}
-if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
-drc->allocation_state = state;
-if (drc->awaiting_release &&
-drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE) {
-uint32_t drc_index = spapr_drc_index(drc);
-trace_spapr_drc_set_allocation_state_finalizing(drc_index);
-spapr_drc_detach(drc, DEVICE(drc->dev), NULL);
-} else if (drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_USABLE) {
-drc->awaiting_allocation = false;
-}
+drc->allocation_state = SPAPR_DR_ALLOCATION_STATE_USABLE;
+drc->awaiting_allocation = false;
+
+return RTAS_OUT_SUCCESS;
+}
+
+static uint32_t drc_set_unusable(sPAPRDRConnector *drc)
+{
+drc->allocation_state = SPAPR_DR_ALLOCATION_STATE_UNUSABLE;
+if (drc->awaiting_release) {
+uint32_t drc_index = spapr_drc_index(drc);
+trace_spapr_drc_set_allocation_state_finalizing(drc_index);
+spapr_drc_detach(drc, DEVICE(drc->dev), NULL);
}
+
return RTAS_OUT_SUCCESS;
}
@@ -553,7 +552,6 @@ static void spapr_dr_connector_class_init(ObjectClass *k,
void *data)
dk->realize = realize;
dk->unrealize = unrealize;
drck->set_isolation_state = set_isolation_state;
-drck->set_allocation_state = set_allocation_state;
drck->release_pending = release_pending;
/*
* Reason: it crashes FIXME find and document the real reason
@@ -823,14 +821,23 @@ static uint32_t rtas_set_isolation_state(uint32_t idx,
uint32_t state)
static uint32_t rtas_set_allocation_state(uint32_t idx, uint32_t state)
{
sPAPRDRConnector *drc = spapr_drc_by_index(idx);
-sPAPRDRConnectorClass *
[Qemu-devel] [PATCH 1/6] spapr: Start hotplugged PCI devices in ISOLATED state
PCI DRCs, and only PCI DRCs, are immediately moved to UNISOLATED isolation
state once the device is attached. This has been there from the initial
implementation, and it's not clear why.
The state diagram in PAPR 13.4 suggests PCI devices should start in
ISOLATED state until the guest moves them into UNISOLATED, and the code in
the guest-side drmgr tool seems to work that way too.
Signed-off-by: David Gibson
Reviewed-by: Michael Roth
---
hw/ppc/spapr_drc.c | 10 --
1 file changed, 10 deletions(-)
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 15ef67d..6186f79 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -315,16 +315,6 @@ void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState
*d, void *fdt,
}
g_assert(fdt || coldplug);
-/* NOTE: setting initial isolation state to UNISOLATED means we can't
- * detach unless guest has a userspace/kernel that moves this state
- * back to ISOLATED in response to an unplug event, or this is done
- * manually by the admin prior. if we force things while the guest
- * may be accessing the device, we can easily crash the guest, so we
- * we defer completion of removal in such cases to the reset() hook.
- */
-if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_PCI) {
-drc->isolation_state = SPAPR_DR_ISOLATION_STATE_UNISOLATED;
-}
drc->dr_indicator = SPAPR_DR_INDICATOR_ACTIVE;
drc->dev = d;
--
2.9.4
[Qemu-devel] [PATCH 0/6] spapr: DRC cleanups (part IV)
This fourth isntallment of cleanups to the DRC code introduces the first changes to the fundamental state handling. We change the initial states in the reset code and attach code for PCI devices, and are able to remove the 'signalled' state variable with those fixes. There are also some more mechanical cleanups in preparation for further cleanups and fixes to the state management. David Gibson (6): spapr: Start hotplugged PCI devices in ISOLATED state spapr: Eliminate DRC 'signalled' state variable spapr: Split DRC release from DRC detach spapr: Make DRC reset force DRC into known state spapr: Clean up DRC set_allocation_state path spapr: Clean up DRC set_isolation_state() path hw/ppc/spapr.c | 15 -- hw/ppc/spapr_drc.c | 363 +++-- hw/ppc/spapr_events.c | 10 -- include/hw/ppc/spapr_drc.h | 10 +- 4 files changed, 188 insertions(+), 210 deletions(-) -- 2.9.4
[Qemu-devel] [PATCH 4/6] spapr: Make DRC reset force DRC into known state
The reset handler for DRCs attempts several state transitions which are
subject to various checks and restrictions. But at reset time we know
there is no guest, so we can ignore most of the usual sequencing rules and
just set the DRC back to a known state. In fact, it's safer to do so.
The existing code also has several redundant checks for
drc->awaiting_release inside a block which has already tested that. This
patch removes those and sets the DRC to a fixed initial state based only
on whether a device is currently plugged or not.
With DRCs correctly reset to a state based on device presence, we don't
need to force state transitions as cold plugged devices are processed.
This allows us to remove all the callers of the set_*_state() methods from
outside spapr_drc.c.
Signed-off-by: David Gibson
---
hw/ppc/spapr.c | 15 ---
hw/ppc/spapr_drc.c | 28
2 files changed, 8 insertions(+), 35 deletions(-)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 01dda9e..c988e38 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2545,12 +2545,6 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t
addr_start, uint64_t size,
spapr_drc_attach(drc, dev, fdt, fdt_offset, !dev->hotplugged, errp);
addr += SPAPR_MEMORY_BLOCK_SIZE;
-if (!dev->hotplugged) {
-sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
-/* guests expect coldplugged LMBs to be pre-allocated */
-drck->set_allocation_state(drc, SPAPR_DR_ALLOCATION_STATE_USABLE);
-drck->set_isolation_state(drc,
SPAPR_DR_ISOLATION_STATE_UNISOLATED);
-}
}
/* send hotplug notification to the
* guest only in case of hotplugged memory
@@ -2901,15 +2895,6 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev,
DeviceState *dev,
* of hotplugged CPUs.
*/
spapr_hotplug_req_add_by_index(drc);
-} else {
-/*
- * Set the right DRC states for cold plugged CPU.
- */
-if (drc) {
-sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
-drck->set_allocation_state(drc, SPAPR_DR_ALLOCATION_STATE_USABLE);
-drck->set_isolation_state(drc,
SPAPR_DR_ISOLATION_STATE_UNISOLATED);
-}
}
core_slot->cpu = OBJECT(dev);
}
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index dc4ac77..7e17f34 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -393,7 +393,6 @@ static bool release_pending(sPAPRDRConnector *drc)
static void reset(DeviceState *d)
{
sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(d);
-sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
trace_spapr_drc_reset(spapr_drc_index(drc));
@@ -401,28 +400,17 @@ static void reset(DeviceState *d)
drc->ccs = NULL;
/* immediately upon reset we can safely assume DRCs whose devices
- * are pending removal can be safely removed, and that they will
- * subsequently be left in an ISOLATED state. move the DRC to this
- * state in these cases (which will in turn complete any pending
- * device removals)
+ * are pending removal can be safely removed.
*/
if (drc->awaiting_release) {
-drck->set_isolation_state(drc, SPAPR_DR_ISOLATION_STATE_ISOLATED);
-/* generally this should also finalize the removal, but if the device
- * hasn't yet been configured we normally defer removal under the
- * assumption that this transition is taking place as part of device
- * configuration. so check if we're still waiting after this, and
- * force removal if we are
- */
-if (drc->awaiting_release) {
-spapr_drc_detach(drc, DEVICE(drc->dev), NULL);
-}
+spapr_drc_release(drc);
+}
-/* non-PCI devices may be awaiting a transition to UNUSABLE */
-if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI &&
-drc->awaiting_release) {
-drck->set_allocation_state(drc,
SPAPR_DR_ALLOCATION_STATE_UNUSABLE);
-}
+drc->isolation_state = drc->dev ? SPAPR_DR_ISOLATION_STATE_UNISOLATED
+: SPAPR_DR_ISOLATION_STATE_ISOLATED;
+if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
+drc->allocation_state = drc->dev ? SPAPR_DR_ALLOCATION_STATE_USABLE
+: SPAPR_DR_ALLOCATION_STATE_UNUSABLE;
}
}
--
2.9.4
[Qemu-devel] [PATCH 3/6] spapr: Split DRC release from DRC detach
spapr_drc_detach() is called when qemu generic code requests a device be
unplugged. It makes a number of tests, which could well delay further
action until later, before actually detach the device from the DRC.
This splits out the part which actually removes the device from the DRC
into spapr_drc_release(). This will be useful for further cleanups.
Signed-off-by: David Gibson
---
hw/ppc/spapr_drc.c | 54 ++
1 file changed, 30 insertions(+), 24 deletions(-)
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index afd68f4..dc4ac77 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -326,31 +326,9 @@ void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState
*d, void *fdt,
NULL, 0, NULL);
}
-void spapr_drc_detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp)
-{
-trace_spapr_drc_detach(spapr_drc_index(drc));
-
-if (drc->isolation_state != SPAPR_DR_ISOLATION_STATE_ISOLATED) {
-trace_spapr_drc_awaiting_isolated(spapr_drc_index(drc));
-drc->awaiting_release = true;
-return;
-}
-
-if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI &&
-drc->allocation_state != SPAPR_DR_ALLOCATION_STATE_UNUSABLE) {
-trace_spapr_drc_awaiting_unusable(spapr_drc_index(drc));
-drc->awaiting_release = true;
-return;
-}
-
-if (drc->awaiting_allocation) {
-if (!drc->awaiting_allocation_skippable) {
-drc->awaiting_release = true;
-trace_spapr_drc_awaiting_allocation(spapr_drc_index(drc));
-return;
-}
-}
+static void spapr_drc_release(sPAPRDRConnector *drc)
+{
drc->dr_indicator = SPAPR_DR_INDICATOR_INACTIVE;
/* Calling release callbacks based on spapr_drc_type(drc). */
@@ -379,6 +357,34 @@ void spapr_drc_detach(sPAPRDRConnector *drc, DeviceState
*d, Error **errp)
drc->dev = NULL;
}
+void spapr_drc_detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp)
+{
+trace_spapr_drc_detach(spapr_drc_index(drc));
+
+if (drc->isolation_state != SPAPR_DR_ISOLATION_STATE_ISOLATED) {
+trace_spapr_drc_awaiting_isolated(spapr_drc_index(drc));
+drc->awaiting_release = true;
+return;
+}
+
+if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI &&
+drc->allocation_state != SPAPR_DR_ALLOCATION_STATE_UNUSABLE) {
+trace_spapr_drc_awaiting_unusable(spapr_drc_index(drc));
+drc->awaiting_release = true;
+return;
+}
+
+if (drc->awaiting_allocation) {
+if (!drc->awaiting_allocation_skippable) {
+drc->awaiting_release = true;
+trace_spapr_drc_awaiting_allocation(spapr_drc_index(drc));
+return;
+}
+}
+
+spapr_drc_release(drc);
+}
+
static bool release_pending(sPAPRDRConnector *drc)
{
return drc->awaiting_release;
--
2.9.4
[Qemu-devel] [PATCH 2/6] spapr: Eliminate DRC 'signalled' state variable
The 'signalled' field in the DRC appears to be entirely a torturous
workaround for the fact that PCI devices were started in UNISOLATED state
for unclear reasons.
1) 'signalled' is already meaningless for logical (so far, all non PCI)
DRCs. It's always set to true (at least at any point it might be tested),
and can't be assigned any real meaning due to the way signalling works for
logical DRCs.
2) For PCI DRCs, the only time signalled would be false is when non-zero
functions of a multifunction device are hotplugged, followed by function
zero (the other way around is explicitly not permitted). In that case the
secondary function DRCs are attached, but the notification isn't sent to
the guest until function 0 is plugged.
3) signalled being false is used to allow a DRC detach to switch mode
back to ISOLATED state, which allows a secondary function to be hotplugged
then unplugged with function 0 never inserted. Without this a secondary
function starting in UNISOLATED state couldn't be detached again without
function 0 being inserted, all the functions configured by the guest, then
sent back to ISOLATED state.
4) But now that PCI DRCs start in ISOLATED state, there's nothing to be
done. If the guest doesn't get the notification, it won't switch the
device to UNISOLATED state, so nothing prevents it from being unplugged.
If the guest does move it to UNISOLATED state without the signal (due to
a manual drmgr call, for instance) then it really isn't safe to unplug it.
So, this patch removes the signalled variable and all code related to it.
Signed-off-by: David Gibson
---
hw/ppc/spapr_drc.c | 45 +
hw/ppc/spapr_events.c | 10 --
include/hw/ppc/spapr_drc.h | 2 --
3 files changed, 1 insertion(+), 56 deletions(-)
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 6186f79..afd68f4 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -183,12 +183,6 @@ static const char *spapr_drc_name(sPAPRDRConnector *drc)
return g_strdup_printf("%s%d", drck->drc_name_prefix, drc->id);
}
-/* has the guest been notified of device attachment? */
-static void set_signalled(sPAPRDRConnector *drc)
-{
-drc->signalled = true;
-}
-
/*
* dr-entity-sense sensor value
* returned via get-sensor-state RTAS calls
@@ -321,17 +315,6 @@ void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState
*d, void *fdt,
drc->fdt = fdt;
drc->fdt_start_offset = fdt_start_offset;
drc->configured = coldplug;
-/* 'logical' DR resources such as memory/cpus are in some cases treated
- * as a pool of resources from which the guest is free to choose from
- * based on only a count. for resources that can be assigned in this
- * fashion, we must assume the resource is signalled immediately
- * since a single hotplug request might make an arbitrary number of
- * such attached resources available to the guest, as opposed to
- * 'physical' DR resources such as PCI where each device/resource is
- * signalled individually.
- */
-drc->signalled = (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI)
- ? true : coldplug;
if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
drc->awaiting_allocation = true;
@@ -347,26 +330,6 @@ void spapr_drc_detach(sPAPRDRConnector *drc, DeviceState
*d, Error **errp)
{
trace_spapr_drc_detach(spapr_drc_index(drc));
-/* if we've signalled device presence to the guest, or if the guest
- * has gone ahead and configured the device (via manually-executed
- * device add via drmgr in guest, namely), we need to wait
- * for the guest to quiesce the device before completing detach.
- * Otherwise, we can assume the guest hasn't seen it and complete the
- * detach immediately. Note that there is a small race window
- * just before, or during, configuration, which is this context
- * refers mainly to fetching the device tree via RTAS.
- * During this window the device access will be arbitrated by
- * associated DRC, which will simply fail the RTAS calls as invalid.
- * This is recoverable within guest and current implementations of
- * drmgr should be able to cope.
- */
-if (!drc->signalled && !drc->configured) {
-/* if the guest hasn't seen the device we can't rely on it to
- * set it back to an isolated state via RTAS, so do it here manually
- */
-drc->isolation_state = SPAPR_DR_ISOLATION_STATE_ISOLATED;
-}
-
if (drc->isolation_state != SPAPR_DR_ISOLATION_STATE_ISOLATED) {
trace_spapr_drc_awaiting_isolated(spapr_drc_index(drc));
drc->awaiting_release = true;
@@ -455,10 +418,6 @@ static void reset(DeviceState *d)
drck->set_allocation_state(drc,
SPAPR_DR_ALLOCATION_STATE_UNUSABLE);
}
}
-
-if (drck->dr_entity_sense(drc) == SPAPR_DR_ENTITY_SENSE_PRESENT) {
-drck->set_signalled(drc);
-}
}
st
Re: [Qemu-devel] [PATCH] nvdimm acpi: fix region format interface code
On 06/08/17 11:07 +0800, Xiao Guangrong wrote: > > > On 06/07/2017 04:06 PM, Haozhong Zhang wrote: > > Per ACPI 6.2, section 5.2.25.6 and JEDEC Annex L Release 3, the > > current region format interface code 0x201 indicates the block > > addressed function interface 1, rather than a byte addressable > > interface. Fix it by using 0x301 which indicates the byte addressable > > no energy backed function interface 1. > > > > Signed-off-by: Haozhong Zhang > > --- > > hw/acpi/nvdimm.c | 7 --- > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c > > index 8e7d6ec034..b5734f5897 100644 > > --- a/hw/acpi/nvdimm.c > > +++ b/hw/acpi/nvdimm.c > > @@ -338,9 +338,10 @@ static void nvdimm_build_structure_dcr(GArray > > *structures, DeviceState *dev) > > nfit_dcr->revision_id = cpu_to_le16(1 /* Current Revision supported > >in ACPI 6.0 is 1. */); > > nfit_dcr->serial_number = cpu_to_le32(sn); > > -nfit_dcr->fic = cpu_to_le16(0x201 /* Format Interface Code. See Chapter > > - 2: NVDIMM Device Specific Method > > - (DSM) in DSM Spec Rev1.*/); > > +nfit_dcr->fic = cpu_to_le16(0x301 /* Format Interface Code: > > + Byte addressable, no energy > > backed. > > + See ACPI 6.2, sect 5.2.25.6 and > > + JEDEC Annex L Release 3. */); > > Shouldn't the 'no energy backend' indicator be set only for !dax disk? Above specs define RFIC for two classes of byte-addressable NVDIMM: 1. 0x10[0|1]: A function containing byte addressable persistent memory whose persistence is achieved through the use of DRAM, nonvolatile memory (e.g., Flash) and an energy source. Only the DRAM portion is addressable by the system. 2. 0x30[0|1]: A function containing byte addressable persistent memory. All of the persistent memory is addressable by the system. No external energy source is required. If the last bit is 0, then it's a proprietary interface. The external backed energy is a mechanism that implements the persistence for the first class, and it's not required for the second class. The key point in this patch is RFIC provided by QEMU should indicate a *byte addressable* NVDIMM. Whether it's backed by the external energy does not matter much here, so I think it's free to choose one of them. Haozhong
Re: [Qemu-devel] [Qemu-ppc] [PATCH] pseries: Correct panic behaviour for pseries machine type
On Thu, Jun 08, 2017 at 06:33:57AM +0200, Thomas Huth wrote:
> On 08.06.2017 02:18, David Gibson wrote:
> > On Wed, Jun 07, 2017 at 07:10:55PM +0200, Thomas Huth wrote:
> >> On 07.06.2017 16:34, Paolo Bonzini wrote:
> >>>
> >>>
> >>> On 07/06/2017 09:33, Thomas Huth wrote:
> On 07.06.2017 09:07, David Gibson wrote:
> > The pseries machine type doesn't usually use the 'pvpanic' device as
> > such,
> > because it has a firmware/hypervisor facility with roughly the same
> > purpose. The 'ibm,os-term' RTAS call notifies the hypervisor that the
> > guest has crashed.
> >
> > Our implementation of this call was sending a GUEST_PANICKED qmp event;
> > however, it was not doing the other usual panic actions, making its
> > behaviour different from pvpanic for no good reason.
> >
> > To correct this, we should call qemu_system_guest_panicked() rather than
> > directly sending the panic event.
> >
> > Signed-off-by: David Gibson
> > ---
> > hw/ppc/spapr_rtas.c | 7 ++-
> > 1 file changed, 2 insertions(+), 5 deletions(-)
> >
> > diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> > index 707c4d4..94a2799 100644
> > --- a/hw/ppc/spapr_rtas.c
> > +++ b/hw/ppc/spapr_rtas.c
> > @@ -293,12 +293,9 @@ static void rtas_ibm_os_term(PowerPCCPU *cpu,
> > target_ulong args,
> > uint32_t nret, target_ulong rets)
> > {
> > -target_ulong ret = 0;
> > +qemu_system_guest_panicked(NULL);
> >
> > -qapi_event_send_guest_panicked(GUEST_PANIC_ACTION_PAUSE, false,
> > NULL,
> > - &error_abort);
> > -
> > -rtas_st(rets, 0, ret);
> > +rtas_st(rets, 0, RTAS_OUT_SUCCESS);
> > }
> >
> > static void rtas_set_power_level(PowerPCCPU *cpu, sPAPRMachineState
> > *spapr,
> >
>
> If I get that qemu_system_guest_panicked() function right, it will stop
> the VM, won't it? That contradicts the LoPAPR spec that says that the
> RTAS call returns if the "ibm,extended-os-term" property is available in
> the device tree.
> >>>
> >>> It does return... but only after the user starts the guest again with
> >>> "cont".
> >>
> >> OK, I guess that's enough to say that the "ibm,extended-os-term"
> >> property can stay ... so I think the patch is fine as it is right now.
> >
> > So.. can I have an R-b?
>
> Reviewed-by: Thomas Huth
Thanks.
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [Qemu-devel] [PATCH v2] Fix the -accel parameter and the documentation for 'hax'
On 07.06.2017 22:14, Emilio G. Cota wrote:
> On Thu, May 04, 2017 at 09:11:50 +0200, Markus Armbruster wrote:
>> Thomas Huth writes:
> (snip)
>>> STEXI
>>> @item -accel @var{name}[,prop=@var{value}[,...]]
>>> @findex -accel
>>> This is used to enable an accelerator. Depending on the target
>>> architecture,
>>> -kvm, xen, or tcg can be available. By default, tcg is used. If there is
>>> more
>>> -than one accelerator specified, the next one is used if the previous one
>>> fails
>>> -to initialize.
>>> +kvm, xen, hax or tcg can be available. By default, tcg is used. If there is
>>> +more than one accelerator specified, the next one is used if the previous
>>> one
>>> +fails to initialize.
>>> @table @option
>>> @item thread=single|multi
>>> Controls number of TCG threads. When the TCG is multi-threaded there will
>>> be one
>>> diff --git a/vl.c b/vl.c
>>> index f46e070..0a1b931 100644
>>> --- a/vl.c
>>> +++ b/vl.c
>>> @@ -3725,26 +3725,21 @@ int main(int argc, char **argv, char **envp)
>>> qdev_prop_register_global(&kvm_pit_lost_tick_policy);
>>> break;
>>> }
>>> -case QEMU_OPTION_accel:
>>> +case QEMU_OPTION_accel: {
>>> +QemuOpts *accel_opts;
>>
>> Doesn't this shadow the @accel_opts declared in main()'s outermost
>> scope?
>
> Yes, it does :( Unfortunately Markus' review slipped through the
> cracks and this patch ended up upstream (bde4d9205). It causes
> a regression that breaks qemu_tcg_configure(accel_opts)
> since now accel_opts is always NULL. That is, in `-accel [..],thread=foo'
> foo is ignored.
Ouch, not sure how I managed to miss that ... big sorry, I'll send a
patch for fixing that mess...
Thomas
Re: [Qemu-devel] [Qemu-ppc] [PATCH] pseries: Correct panic behaviour for pseries machine type
On 08.06.2017 02:18, David Gibson wrote:
> On Wed, Jun 07, 2017 at 07:10:55PM +0200, Thomas Huth wrote:
>> On 07.06.2017 16:34, Paolo Bonzini wrote:
>>>
>>>
>>> On 07/06/2017 09:33, Thomas Huth wrote:
On 07.06.2017 09:07, David Gibson wrote:
> The pseries machine type doesn't usually use the 'pvpanic' device as such,
> because it has a firmware/hypervisor facility with roughly the same
> purpose. The 'ibm,os-term' RTAS call notifies the hypervisor that the
> guest has crashed.
>
> Our implementation of this call was sending a GUEST_PANICKED qmp event;
> however, it was not doing the other usual panic actions, making its
> behaviour different from pvpanic for no good reason.
>
> To correct this, we should call qemu_system_guest_panicked() rather than
> directly sending the panic event.
>
> Signed-off-by: David Gibson
> ---
> hw/ppc/spapr_rtas.c | 7 ++-
> 1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index 707c4d4..94a2799 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -293,12 +293,9 @@ static void rtas_ibm_os_term(PowerPCCPU *cpu,
> target_ulong args,
> uint32_t nret, target_ulong rets)
> {
> -target_ulong ret = 0;
> +qemu_system_guest_panicked(NULL);
>
> -qapi_event_send_guest_panicked(GUEST_PANIC_ACTION_PAUSE, false, NULL,
> - &error_abort);
> -
> -rtas_st(rets, 0, ret);
> +rtas_st(rets, 0, RTAS_OUT_SUCCESS);
> }
>
> static void rtas_set_power_level(PowerPCCPU *cpu, sPAPRMachineState
> *spapr,
>
If I get that qemu_system_guest_panicked() function right, it will stop
the VM, won't it? That contradicts the LoPAPR spec that says that the
RTAS call returns if the "ibm,extended-os-term" property is available in
the device tree.
>>>
>>> It does return... but only after the user starts the guest again with
>>> "cont".
>>
>> OK, I guess that's enough to say that the "ibm,extended-os-term"
>> property can stay ... so I think the patch is fine as it is right now.
>
> So.. can I have an R-b?
Reviewed-by: Thomas Huth
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH] Makefile: Do not generate files if "configure" has not been run yet
On 07.06.2017 22:59, Eric Blake wrote: > On 06/07/2017 02:11 PM, Thomas Huth wrote: >> When doing a "make -j10" in the vanilla QEMU source tree (without >> running "configure first), the Makefile currently generates two > > s/"configure/"configure"/ > >> files already, qemu-version.h and qemu-options.def. This should not >> happen, so let's make these targets depend on config-host.mak. >> Also the python files can not be executed without $(PYTHON), so >> these scripts should depend on config-host.mak, too. >> >> Signed-off-by: Thomas Huth >> --- >> Makefile | 5 - >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/Makefile b/Makefile >> index c830d7a..6786dc2 100644 >> --- a/Makefile >> +++ b/Makefile >> @@ -286,7 +286,7 @@ endif >> >> all: $(DOCS) $(TOOLS) $(HELPERS-y) recurse-all modules >> >> -qemu-version.h: FORCE >> +qemu-version.h: config-host.mak FORCE > > This one makes sense. > >> @@ -393,6 +394,8 @@ gen-out-type = $(subst .,-,$(suffix $@)) >> >> qapi-py = $(SRC_PATH)/scripts/qapi.py $(SRC_PATH)/scripts/ordereddict.py >> >> +$(qapi-py): config-host.mak > > But this one is weird. How can a pre-existing file have a dependency? > Remember, $(qapi-py) is not the list of built files, but the list of > files used to build other files. It seems like you either want > config-host.mak includes in $(qapi-py), or... > >> + >> qga/qapi-generated/qga-qapi-types.c qga/qapi-generated/qga-qapi-types.h :\ >> $(SRC_PATH)/qga/qapi-schema.json $(SRC_PATH)/scripts/qapi-types.py >> $(qapi-py) > > ...that THIS should be the rule that depends on config-host.mak in > addition do depending on $(qapi-py). Yes, it's all those targets that use $(PYTHON) which should depend on config-host.mak instead. But there are lots of them, and all of them depend on $(qapi-py) already, so it seemed simpler to make $(qapy-py) depend on config-host.mak instead of adding that dependency to all those targets ... but maybe that's too confusing in the long run, so I'll add it to those targets instead. Thomas signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v5 1/2] spapr: Add a "no HPT" encoding to HTAB migration stream
On Wed, Jun 07, 2017 at 01:24:52PM +0530, Bharata B Rao wrote:
> Add a "no HPT" encoding (using value -1) to the HTAB migration
> stream (in the place of HPT size) when the guest doesn't allocate HPT.
> This will help the target side to match target HPT with the source HPT
> and thus enable successful migration.
>
> Suggested-by: David Gibson
> Signed-off-by: Bharata B Rao
As dicussed in the thread on the previous version, I still think you
can use section_hdr == 0 as the no-HPT encoding (matching htab_shift
== 0.
> ---
> hw/ppc/spapr.c | 28
> 1 file changed, 24 insertions(+), 4 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 86e6228..df27c5c 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1557,13 +1557,19 @@ static int htab_save_setup(QEMUFile *f, void *opaque)
> sPAPRMachineState *spapr = opaque;
>
> /* "Iteration" header */
> -qemu_put_be32(f, spapr->htab_shift);
> +if (!spapr->htab_shift) {
> +qemu_put_be32(f, -1);
> +} else {
> +qemu_put_be32(f, spapr->htab_shift);
> +}
>
> if (spapr->htab) {
> spapr->htab_save_index = 0;
> spapr->htab_first_pass = true;
> } else {
> -assert(kvm_enabled());
> +if (spapr->htab_shift) {
> +assert(kvm_enabled());
> +}
> }
>
>
> @@ -1709,7 +1715,12 @@ static int htab_save_iterate(QEMUFile *f, void *opaque)
> int rc = 0;
>
> /* Iteration header */
> -qemu_put_be32(f, 0);
> +if (!spapr->htab_shift) {
> +qemu_put_be32(f, -1);
> +return 0;
> +} else {
> +qemu_put_be32(f, 0);
> +}
>
> if (!spapr->htab) {
> assert(kvm_enabled());
> @@ -1743,7 +1754,12 @@ static int htab_save_complete(QEMUFile *f, void
> *opaque)
> int fd;
>
> /* Iteration header */
> -qemu_put_be32(f, 0);
> +if (!spapr->htab_shift) {
> +qemu_put_be32(f, -1);
> +return 0;
> +} else {
> +qemu_put_be32(f, 0);
> +}
>
> if (!spapr->htab) {
> int rc;
> @@ -1787,6 +1803,10 @@ static int htab_load(QEMUFile *f, void *opaque, int
> version_id)
>
> section_hdr = qemu_get_be32(f);
>
> +if (section_hdr == -1) {
You should free the existing HPT (if any) here.
> +return 0;
> +}
> +
> if (section_hdr) {
> Error *local_err = NULL;
>
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [Qemu-devel] [PATCH] hw/ppc/spapr: Adjust firmware name for PCI bridges
On Wed, Jun 07, 2017 at 10:20:27AM +0200, Thomas Huth wrote:
> SLOF uses "pci" as name for PCI bridges nodes in the device tree instead
> of "pci-bridges", so booting via bootindex from a device behind a PCI
> bridge currently does not work since QEMU passes the wrong name in the
> "qemu,boot-list" property. Fix it by changing the name of the PCI bridge
> nodes to "pci" instead.
>
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1459170
> Signed-off-by: Thomas Huth
Applied to ppc-for-2.10.
> ---
> hw/ppc/spapr.c | 6 ++
> 1 file changed, 6 insertions(+)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 91b4057..27b1f3c 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2441,6 +2441,12 @@ static char *spapr_get_fw_dev_path(FWPathProvider *p,
> BusState *bus,
> return g_strdup_printf("disk@%"PRIX64, (uint64_t)id << 32);
> }
>
> +if (g_str_equal("pci-bridge", qdev_fw_name(dev))) {
> +/* SLOF uses "pci" instead of "pci-bridge" for PCI bridges */
> +PCIDevice *pcidev = CAST(PCIDevice, dev, TYPE_PCI_DEVICE);
> +return g_strdup_printf("pci@%x", PCI_SLOT(pcidev->devfn));
> +}
> +
> return NULL;
> }
>
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [Qemu-devel] [PATCHv5 0/4] Clean up compatibility mode handling
On Fri, Jun 02, 2017 at 01:15:03PM +1000, David Gibson wrote: > This is a rebased and revised version of my patches revising CPU > compatiblity mode handling on ppc, last posted in November. Since > then, many of the patches have already been merged (some for 2.9, some > since). This is what's left. > > * There was conceptual confusion about what a compatibility mode >means, and how it interacts with the machine type. This cleans >that up, clarifying that a compatibility mode (as an externally set >option) only makes sense on machine types that don't permit the >guest hypervisor privilege (i.e. 'pseries') > > * It was previously the user's (or management layer's) responsibility >to determine compatibility of CPUs on either end for migration. >This uses the compatibility modes to check that properly during an >incoming migration. Anyone willing to give some review and/or testing, particularly for patch 2/4. -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
Re: [Qemu-devel] [PATCH v4 1/2] spapr: Add a "no HPT" encoding to HTAB migration stream
On Wed, Jun 07, 2017 at 01:23:32PM +0530, Bharata B Rao wrote:
> On Thu, Jun 01, 2017 at 02:54:48PM +1000, David Gibson wrote:
> > On Wed, May 31, 2017 at 04:56:44PM +0530, Bharata B Rao wrote:
> > > Add a "no HPT" encoding (using value -1) to the HTAB migration
> > > stream (in the place of HPT size) when the guest doesn't allocate HPT.
> > > This will help the target side to match target HPT with the source HPT
> > > and thus enable successful migration.
> > >
> > > A few more fixes to enable TCG migration to work correctly are also
> > > included in this commit:
> > >
> > > - HTAB savevm handlers have a few asserts on kvm_enabled() when
> > > spapr->htab != 0. Convert these into conditional checks as it is now
> > > possible to have no HTAB with TCG radix guests.
> > > - htab_save_setup() asserts for kvm_enabled() when spapr->htab != 0.
> > > Remove this as we can't assert this for TCG radix guests.
> > >
> > > Suggested-by: David Gibson
> > > [no HPT encoding suggestion]
> > > Signed-off-by: Bharata B Rao
> >
> > Looks basically ok, but there are still some details to address.
> >
> > > ---
> > > hw/ppc/spapr.c | 31 +--
> > > 1 file changed, 17 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index ab3aab1..b589ed4 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -1559,17 +1559,18 @@ static int htab_save_setup(QEMUFile *f, void
> > > *opaque)
> > > {
> > > sPAPRMachineState *spapr = opaque;
> > >
> > > -/* "Iteration" header */
> > > -qemu_put_be32(f, spapr->htab_shift);
> > > +/* "Iteration" header: no-HPT or HPT size encoding */
> > > +if (!spapr->htab_shift) {
> > > +qemu_put_be32(f, -1);
> >
> > We're already using htab_shift == 0 to represent no HPT in the runtime
> > structure; we might as well do the same on the wire. As a bonus it
> > slightly simplifies the logic here.
>
> Non-zero value of iteration header (which is htab_shift) results in
> htab_load() at the target to reallocate HTAB.
Yeah.. but you can change that, right. Have htab_load() remove the
HPT instead, if section_hdr == 0.
Older qemus that don't understand that certainly weren't going to be
able to take an incoming RPT guest anyway.
> zero value of iteration header is used by htab_save_iterate() and
> htab_save_complete() to tell htab_load() not to freshly allocate HTAB
> at the target.
But that makes no sense anyway. How the destination goes about
allocating or not allocating the HPT shouldn't be the choice of the
source. Since the source doesn't know what the destination has
already allocated, it can't possibly know if it's of a matching size,
likewise if the destination doesn't get a size, it can't know if the
current size is correct.
> Hence we can't use 0 value to mean no-HPT.
I really think we can..
> I have addressed the rest of the comments on asserts by ensuring that
> those code paths are taken only when HPT is present. v5 has those
> changes.
>
> Regards,
> Bharata.
>
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [Qemu-devel] [PATCH v3 5/5] spapr: fix migration of ICPState objects from/to older QEMU
On Wed, Jun 07, 2017 at 07:17:26PM +0200, Greg Kurz wrote:
> Commit 5bc8d26de20c ("spapr: allocate the ICPState object from under
> sPAPRCPUCore") moved ICPState objects from the machine to CPU cores.
> This is an improvement since we no longer allocate ICPState objects
> that will never be used. But it has the side-effect of breaking
> migration of older machine types from older QEMU versions.
>
> This patch allows spapr to register dummy "icp/server" entries to vmstate.
> These entries use a dedicated VMStateDescription that can swallow and
> discard state of an incoming migration stream, and that don't send anything
> on outgoing migration.
>
> As for real ICPState objects, the instance_id is the cpu_index of the
> corresponding vCPU, which happens to be equal to the generated instance_id
> of older machine types.
>
> The machine can unregister/register these entries when CPUs are dynamically
> plugged/unplugged.
>
> This is only available for pseries-2.9 and older machines, thanks to a
> compat property.
>
> Signed-off-by: Greg Kurz
> ---
> v3: - new logic entirely implemented in hw/ppc/spapr.c
> ---
> hw/ppc/spapr.c | 88
> +++-
> include/hw/ppc/spapr.h |2 +
> 2 files changed, 88 insertions(+), 2 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 9b7ae28939a8..c15b604978f0 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -124,9 +124,52 @@ error:
> return NULL;
> }
>
> +static bool pre_2_10_vmstate_dummy_icp_needed(void *opaque)
> +{
> +return false;
> +}
Uh.. the needed function always returns false, how can that work?
> +
> +static const VMStateDescription pre_2_10_vmstate_dummy_icp = {
> +.name = "icp/server",
> +.version_id = 1,
> +.minimum_version_id = 1,
> +.needed = pre_2_10_vmstate_dummy_icp_needed,
> +.fields = (VMStateField[]) {
> +VMSTATE_UNUSED(4), /* uint32_t xirr */
> +VMSTATE_UNUSED(1), /* uint8_t pending_priority */
> +VMSTATE_UNUSED(1), /* uint8_t mfrr */
> +VMSTATE_END_OF_LIST()
> +},
> +};
> +
> +static void pre_2_10_vmstate_register_dummy_icp(sPAPRMachineState *spapr,
> int i)
> +{
> +bool *flag = &spapr->pre_2_10_ignore_icp[i];
> +
> +g_assert(!*flag);
Apart from this assert(), you never seem to test the values in the
pre_2_10_ignore_icp() array, so it seems a bit pointless.
> +vmstate_register(NULL, i, &pre_2_10_vmstate_dummy_icp, flag);
> +*flag = true;
> +}
> +
> +static void pre_2_10_vmstate_unregister_dummy_icp(sPAPRMachineState *spapr,
> + int i)
> +{
> +bool *flag = &spapr->pre_2_10_ignore_icp[i];
> +
> +g_assert(*flag);
> +vmstate_unregister(NULL, &pre_2_10_vmstate_dummy_icp, flag);
> +*flag = false;
> +}
> +
> +static inline int xics_nr_servers(void)
Maybe a different name to emphasise that this is only used for the
backwards compat logic.
> +{
> +return DIV_ROUND_UP(max_cpus * kvmppc_smt_threads(), smp_threads);
> +}
> +
> static void xics_system_init(MachineState *machine, int nr_irqs, Error
> **errp)
> {
> sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
> +sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(machine);
>
> if (kvm_enabled()) {
> if (machine_kernel_irqchip_allowed(machine) &&
> @@ -148,6 +191,15 @@ static void xics_system_init(MachineState *machine, int
> nr_irqs, Error **errp)
> return;
> }
> }
> +
> +if (smc->pre_2_10_has_unused_icps) {
> +int i;
> +
> +spapr->pre_2_10_ignore_icp = g_malloc(xics_nr_servers());
> +for (i = 0; i < xics_nr_servers(); i++) {
> +pre_2_10_vmstate_register_dummy_icp(spapr, i);
This registers a dummy ICP for every slot, some of which will have
real cpus / icps. That doesn't seem right.
> +}
> +}
> }
>
> static int spapr_fixup_cpu_smt_dt(void *fdt, int offset, PowerPCCPU *cpu,
> @@ -976,7 +1028,6 @@ static void *spapr_build_fdt(sPAPRMachineState *spapr,
> void *fdt;
> sPAPRPHBState *phb;
> char *buf;
> -int smt = kvmppc_smt_threads();
>
> fdt = g_malloc0(FDT_MAX_SIZE);
> _FDT((fdt_create_empty_tree(fdt, FDT_MAX_SIZE)));
> @@ -1016,7 +1067,7 @@ static void *spapr_build_fdt(sPAPRMachineState *spapr,
> _FDT(fdt_setprop_cell(fdt, 0, "#size-cells", 2));
>
> /* /interrupt controller */
> -spapr_dt_xics(DIV_ROUND_UP(max_cpus * smt, smp_threads), fdt,
> PHANDLE_XICP);
> +spapr_dt_xics(xics_nr_servers(), fdt, PHANDLE_XICP);
>
> ret = spapr_populate_memory(spapr, fdt);
> if (ret < 0) {
> @@ -2800,9 +2851,24 @@ static void spapr_core_unplug(HotplugHandler
> *hotplug_dev, DeviceState *dev,
>Error **errp)
> {
> MachineState *ms = MACHINE(qdev_get_machine());
> +sPAPRMachineState *spapr = SPAPR_MACHINE(ms);
> CPUCore *cc = CPU_CORE(dev);
> CPUArchId *core_slot = sp
Re: [Qemu-devel] [PATCH v3 4/5] xics: directly register ICPState objects to vmstate
On Wed, Jun 07, 2017 at 07:17:17PM +0200, Greg Kurz wrote:
> The ICPState objects are currently registered to vmstate as qdev objects.
> Their instance ids are hence computed automatically in the migration code,
> and thus depends on the order the CPU cores were plugged.
>
> If the destination had its CPU cores plugged in a different order than the
> source, then ICPState objects will have different instance_ids and load
> the wrong state.
>
> Since CPU objects have a reliable cpu_index which is already used as
> instance_id in vmstate, let's use it for ICPState as well.
>
> Signed-off-by: Greg Kurz
> ---
> hw/intc/xics.c |5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
So, this is definitely a good idea w.r.t. future compatibility. But,
won't it break migration compat as a once off, since the devices will
be found under their new id instead of the qdev id?
>
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index 330441ff7fe8..3d76b43876c5 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -348,10 +348,14 @@ static void icp_realize(DeviceState *dev, Error **errp)
> }
>
> qemu_register_reset(icp_reset, dev);
> +vmstate_register(NULL, icp->cs->cpu_index, &vmstate_icp_server, icp);
> }
>
> static void icp_unrealize(DeviceState *dev, Error **errp)
> {
> +ICPState *icp = ICP(dev);
> +
> +vmstate_unregister(NULL, &vmstate_icp_server, icp);
> qemu_unregister_reset(icp_reset, dev);
> }
>
> @@ -359,7 +363,6 @@ static void icp_class_init(ObjectClass *klass, void *data)
> {
> DeviceClass *dc = DEVICE_CLASS(klass);
>
> -dc->vmsd = &vmstate_icp_server;
> dc->realize = icp_realize;
> dc->unrealize = icp_unrealize;
> }
>
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
[Qemu-devel] [PATCH] net/colo-compare.c: Add no-need-checkpoint period to optimize performance
If colo-compare find out the first different packet that means
the following packet almost is different. we needn't do a lot
of checkpoint in one second, so we set the no-need-checkpoint
period, default just set 3 second.
Signed-off-by: Zhang Chen
---
net/colo-compare.c | 9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/net/colo-compare.c b/net/colo-compare.c
index 6d500e1..8fb1c9c 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -39,6 +39,8 @@
/* TODO: Should be configurable */
#define REGULAR_PACKET_CHECK_MS 3000
+/* TODO: Should be configurable */
+#define CHECKPOINT_MIN_TIME 3000
/*
+ CompareState ++
@@ -455,6 +457,7 @@ static void colo_compare_connection(void *opaque, void
*user_data)
Packet *pkt = NULL;
GList *result = NULL;
int ret;
+static int64_t checkpoint_time_ms;
while (!g_queue_is_empty(&conn->primary_list) &&
!g_queue_is_empty(&conn->secondary_list)) {
@@ -494,7 +497,11 @@ static void colo_compare_connection(void *opaque, void
*user_data)
*/
trace_colo_compare_main("packet different");
g_queue_push_tail(&conn->primary_list, pkt);
-/* TODO: colo_notify_checkpoint();*/
+
+if (pkt->creation_ms - checkpoint_time_ms > CHECKPOINT_MIN_TIME) {
+/* TODO: colo_notify_checkpoint();*/
+checkpoint_time_ms = pkt->creation_ms;
+}
break;
}
}
--
2.7.4
[Qemu-devel] [PATCH] util/qemu-sockets: Drop unused helper socket_address_to_string()
Signed-off-by: Mao Zhongyi
---
include/qemu/sockets.h | 15 ---
util/qemu-sockets.c| 34 --
2 files changed, 49 deletions(-)
diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
index 7abffc4..8eb0172 100644
--- a/include/qemu/sockets.h
+++ b/include/qemu/sockets.h
@@ -106,21 +106,6 @@ SocketAddress *socket_local_address(int fd, Error **errp);
SocketAddress *socket_remote_address(int fd, Error **errp);
/**
- * socket_address_to_string:
- * @addr: the socket address struct
- * @errp: pointer to uninitialized error object
- *
- * Get the string representation of the socket
- * address. A pointer to the char array containing
- * string format will be returned, the caller is
- * required to release the returned value when no
- * longer required with g_free.
- *
- * Returns: the socket address in string format, or NULL on error
- */
-char *socket_address_to_string(struct SocketAddress *addr, Error **errp);
-
-/**
* socket_address_flatten:
* @addr: the socket address to flatten
*
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index b39ae74..08b1a26 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -1301,40 +1301,6 @@ SocketAddress *socket_remote_address(int fd, Error
**errp)
return socket_sockaddr_to_address(&ss, sslen, errp);
}
-char *socket_address_to_string(struct SocketAddress *addr, Error **errp)
-{
-char *buf;
-InetSocketAddress *inet;
-
-switch (addr->type) {
-case SOCKET_ADDRESS_TYPE_INET:
-inet = &addr->u.inet;
-if (strchr(inet->host, ':') == NULL) {
-buf = g_strdup_printf("%s:%s", inet->host, inet->port);
-} else {
-buf = g_strdup_printf("[%s]:%s", inet->host, inet->port);
-}
-break;
-
-case SOCKET_ADDRESS_TYPE_UNIX:
-buf = g_strdup(addr->u.q_unix.path);
-break;
-
-case SOCKET_ADDRESS_TYPE_FD:
-buf = g_strdup(addr->u.fd.str);
-break;
-
-case SOCKET_ADDRESS_TYPE_VSOCK:
-buf = g_strdup_printf("%s:%s",
- addr->u.vsock.cid,
- addr->u.vsock.port);
-break;
-
-default:
-abort();
-}
-return buf;
-}
SocketAddress *socket_address_flatten(SocketAddressLegacy *addr_legacy)
{
--
2.9.3
Re: [Qemu-devel] [PATCH] nvdimm acpi: fix region format interface code
On 06/07/2017 04:06 PM, Haozhong Zhang wrote: Per ACPI 6.2, section 5.2.25.6 and JEDEC Annex L Release 3, the current region format interface code 0x201 indicates the block addressed function interface 1, rather than a byte addressable interface. Fix it by using 0x301 which indicates the byte addressable no energy backed function interface 1. Signed-off-by: Haozhong Zhang --- hw/acpi/nvdimm.c | 7 --- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c index 8e7d6ec034..b5734f5897 100644 --- a/hw/acpi/nvdimm.c +++ b/hw/acpi/nvdimm.c @@ -338,9 +338,10 @@ static void nvdimm_build_structure_dcr(GArray *structures, DeviceState *dev) nfit_dcr->revision_id = cpu_to_le16(1 /* Current Revision supported in ACPI 6.0 is 1. */); nfit_dcr->serial_number = cpu_to_le32(sn); -nfit_dcr->fic = cpu_to_le16(0x201 /* Format Interface Code. See Chapter - 2: NVDIMM Device Specific Method - (DSM) in DSM Spec Rev1.*/); +nfit_dcr->fic = cpu_to_le16(0x301 /* Format Interface Code: + Byte addressable, no energy backed. + See ACPI 6.2, sect 5.2.25.6 and + JEDEC Annex L Release 3. */); Shouldn't the 'no energy backend' indicator be set only for !dax disk?
Re: [Qemu-devel] [PATCH v3 3/5] xics: setup cpu at realize time
On Wed, Jun 07, 2017 at 10:55:07PM +0200, Greg Kurz wrote: > On Wed, 7 Jun 2017 20:11:38 +0200 > Cédric Le Goater wrote: > > > On 06/07/2017 07:17 PM, Greg Kurz wrote: > > > Until recently, spapr used to allocate ICPState objects for the lifetime > > > of the machine. They would only be associated to vCPUs in xics_cpu_setup() > > > when plugging a CPU core. > > > > > > Now that ICPState objects have the same lifecycle as vCPUs, it is > > > possible to associate them during realization. > > > > > > This patch hence open-codes xics_cpu_setup() in icp_realize(). The vCPU > > > is passed as a property. Note that vCPU now needs to be realized first > > > for the IRQs to be allocated. It also needs to resetted before ICPState > > > realization in order to synchronize with KVM. > > > > > > Since ICPState objects are freed when unrealized, xics_cpu_destroy() isn't > > > needed anymore and can be safely dropped. > > > > I like the idea but I think the assignment of ->cs attribute should be > > moved under icp_kvm_cpu_setup(), as it is only used under xics_kvm by > > the kvm_vcpu_ioctl() calls. > > > > Well, cs->cpu_index is also used for traces and to implement the 'info pic' > monitor command. Right. I think it makes sense for the ICP to have a handle on it's associated CPU, even if we don't actually use it in all cases right now. So I have no problem with the property being in all ICPs; I think that will be cleaner than special casing xics_kvm. Especially if we have to un-special-case it sometime in future because we need to access the CPU object for some reason we haven't thought of yet. -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
Re: [Qemu-devel] [PATCH v3 2/5] xics: add reset() handler to ICPStateClass
On Wed, Jun 07, 2017 at 07:47:04PM +0200, Cédric Le Goater wrote: 1;4602;0c> On 06/07/2017 07:17 PM, Greg Kurz wrote: > > Taking into account that qemu_set_irq() returns immediatly if its first > > argument is NULL, icp_kvm_reset() largely duplicates icp_reset(). > > > > This patch introduces a reset() handler, so that the common logic can > > be implemented in icp_reset() only. > > > > While there we can also drop icp_kvm_realize() and icp_kvm_unrealize(). This > > causes icp-kvm to be realized in icp_realize(), which sets icp->xics, but > > it has no impact. > > > > Signed-off-by: Greg Kurz > > > Reviewed-by: Cédric Le Goater > > > --- > > hw/intc/xics.c|5 + > > hw/intc/xics_kvm.c| 27 ++- > > include/hw/ppc/xics.h |1 + > > 3 files changed, 8 insertions(+), 25 deletions(-) > > another good cleanup ! I concur. Applied to ppc-for-2.10. -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
Re: [Qemu-devel] [PATCH v3 1/5] pnv_core: drop reference on ICPState object during CPU realization
On Wed, Jun 07, 2017 at 07:49:14PM +0200, Cédric Le Goater wrote:
> On 06/07/2017 07:16 PM, Greg Kurz wrote:
> > Similarly to what was done to spapr with commit 249127d0dfeb, this patch
> > ensures that we don't keep an extra reference on the ICPState object. Also
> > since the object was just created and not reparented yet, the call to
> > object_property_add_child() should never fail: let's pass &error_abort to
> > make this clear.
> >
> > Signed-off-by: Greg Kurz
>
> Reviewed-by: Cédric Le Goater
Applied to ppc-for-2.10, thanks.
>
> Thanks,
>
> C.
>
> > ---
> > hw/ppc/pnv_core.c |3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
> > index 1b7ec70f033d..e8a9a94d5a24 100644
> > --- a/hw/ppc/pnv_core.c
> > +++ b/hw/ppc/pnv_core.c
> > @@ -119,7 +119,8 @@ static void pnv_core_realize_child(Object *child,
> > XICSFabric *xi, Error **errp)
> > Object *obj;
> >
> > obj = object_new(TYPE_PNV_ICP);
> > -object_property_add_child(OBJECT(cpu), "icp", obj, NULL);
> > +object_property_add_child(OBJECT(cpu), "icp", obj, &error_abort);
> > +object_unref(obj);
> > object_property_add_const_link(obj, "xics", OBJECT(xi), &error_abort);
> > object_property_set_bool(obj, true, "realized", &local_err);
> > if (local_err) {
> >
>
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [Qemu-devel] [PATCH v3 3/5] xics: setup cpu at realize time
On Wed, Jun 07, 2017 at 07:17:09PM +0200, Greg Kurz wrote:
> Until recently, spapr used to allocate ICPState objects for the lifetime
> of the machine. They would only be associated to vCPUs in xics_cpu_setup()
> when plugging a CPU core.
>
> Now that ICPState objects have the same lifecycle as vCPUs, it is
> possible to associate them during realization.
>
> This patch hence open-codes xics_cpu_setup() in icp_realize(). The vCPU
> is passed as a property. Note that vCPU now needs to be realized first
> for the IRQs to be allocated. It also needs to resetted before ICPState
> realization in order to synchronize with KVM.
Ok, what enforces those ordering constraints?
> Since ICPState objects are freed when unrealized, xics_cpu_destroy() isn't
> needed anymore and can be safely dropped.
>
> Signed-off-by: Greg Kurz
Looks pretty good, though a couple nits below.
> ---
> hw/intc/xics.c | 76
> ---
> hw/ppc/pnv_core.c | 16 --
> hw/ppc/spapr_cpu_core.c | 21 ++---
> include/hw/ppc/xics.h |2 -
> 4 files changed, 48 insertions(+), 67 deletions(-)
>
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index ec73f02144c9..330441ff7fe8 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -38,50 +38,6 @@
> #include "monitor/monitor.h"
> #include "hw/intc/intc.h"
>
> -void xics_cpu_destroy(XICSFabric *xi, PowerPCCPU *cpu)
> -{
> -CPUState *cs = CPU(cpu);
> -ICPState *icp = ICP(cpu->intc);
> -
> -assert(icp);
> -assert(cs == icp->cs);
> -
> -icp->output = NULL;
> -icp->cs = NULL;
> -}
> -
> -void xics_cpu_setup(XICSFabric *xi, PowerPCCPU *cpu, ICPState *icp)
> -{
> -CPUState *cs = CPU(cpu);
> -CPUPPCState *env = &cpu->env;
> -ICPStateClass *icpc;
> -
> -assert(icp);
> -
> -cpu->intc = OBJECT(icp);
> -icp->cs = cs;
> -
> -icpc = ICP_GET_CLASS(icp);
> -if (icpc->cpu_setup) {
> -icpc->cpu_setup(icp, cpu);
> -}
> -
> -switch (PPC_INPUT(env)) {
> -case PPC_FLAGS_INPUT_POWER7:
> -icp->output = env->irq_inputs[POWER7_INPUT_INT];
> -break;
> -
> -case PPC_FLAGS_INPUT_970:
> -icp->output = env->irq_inputs[PPC970_INPUT_INT];
> -break;
> -
> -default:
> -error_report("XICS interrupt controller does not support this CPU "
> - "bus model");
> -abort();
> -}
> -}
> -
> void icp_pic_print_info(ICPState *icp, Monitor *mon)
> {
> int cpu_index = icp->cs ? icp->cs->cpu_index : -1;
> @@ -343,6 +299,8 @@ static void icp_realize(DeviceState *dev, Error **errp)
> {
> ICPState *icp = ICP(dev);
> ICPStateClass *icpc = ICP_GET_CLASS(dev);
> +PowerPCCPU *cpu;
> +CPUPPCState *env;
> Object *obj;
> Error *err = NULL;
>
> @@ -355,6 +313,36 @@ static void icp_realize(DeviceState *dev, Error **errp)
>
> icp->xics = XICS_FABRIC(obj);
>
> +obj = object_property_get_link(OBJECT(dev), "cs", &err);
I don't like the name "cs" for an externally visible property. "cpu"
would be better.
> +if (!obj) {
> +error_setg(errp, "%s: required link 'xics' not found: %s",
Copy/paste mistake in this message.
> + __func__, error_get_pretty(err));
> +return;
> +}
> +
> +cpu = POWERPC_CPU(obj);
> +cpu->intc = OBJECT(icp);
> +icp->cs = CPU(obj);
> +
> +if (icpc->cpu_setup) {
> +icpc->cpu_setup(icp, cpu);
> +}
> +
> +env = &cpu->env;
> +switch (PPC_INPUT(env)) {
> +case PPC_FLAGS_INPUT_POWER7:
> +icp->output = env->irq_inputs[POWER7_INPUT_INT];
> +break;
> +
> +case PPC_FLAGS_INPUT_970:
> +icp->output = env->irq_inputs[PPC970_INPUT_INT];
> +break;
> +
> +default:
> +error_setg(errp, "XICS interrupt controller does not support this
> CPU bus model");
> +return;
> +}
> +
> if (icpc->realize) {
> icpc->realize(dev, errp);
> }
> diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
> index e8a9a94d5a24..1393005e76f3 100644
> --- a/hw/ppc/pnv_core.c
> +++ b/hw/ppc/pnv_core.c
> @@ -118,19 +118,19 @@ static void pnv_core_realize_child(Object *child,
> XICSFabric *xi, Error **errp)
> PowerPCCPU *cpu = POWERPC_CPU(cs);
> Object *obj;
>
> -obj = object_new(TYPE_PNV_ICP);
> -object_property_add_child(OBJECT(cpu), "icp", obj, &error_abort);
> -object_unref(obj);
> -object_property_add_const_link(obj, "xics", OBJECT(xi), &error_abort);
> -object_property_set_bool(obj, true, "realized", &local_err);
> +object_property_set_bool(child, true, "realized", &local_err);
> if (local_err) {
> error_propagate(errp, local_err);
> return;
> }
>
> -object_property_set_bool(child, true, "realized", &local_err);
> +obj = object_new(TYPE_PNV_ICP);
> +object_property_add_child(child, "icp", obj, NULL);
> +object_unref(obj);
> +object_property_add_cons
Re: [Qemu-devel] [PATCH v3 00/43] qobject/qapi: add uint type
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 20170607163635.17635-1-marcandre.lur...@redhat.com
Subject: [Qemu-devel] [PATCH v3 00/43] qobject/qapi: add uint type
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
git config --local diff.renamelimit 0
git config --local diff.renames True
commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
5304b17 qobject: move dump_qobject() from block/ to qobject/
04e93f9 RFC: qdict: add uint
b1f8183 tests/qdict: check more get_try_int() cases
32dc113 console: use get_uint() for "head" property
ccdbdd6 i386/cpu: use get_uint() for "min-level"/"min-xlevel" properties
eabad51 numa: use get_uint() for "size" property
23ad49e pnv-core: use get_uint() for "core-pir" property
c327b58 pvpanic: use get_uint() for "ioport" property
2af440d auxbus: use get_uint() for "addr" property
63db11b arm: use get_uint() for "mp-affinity" property
ede6718 xen: use get_uint() for "max-ram-below-4g" property
d00a748 pc: use get_uint() for "hpet-intcap" property
2be46be pc: use get_uint() for "apic-id" property
0bbec76 pc: use get_uint() for "iobase" property
54613bf acpi: use get_uint() for "pci-hole*" properties
a9029ef acpi: use get_uint() for various acpi properties
4c2ac9c acpi: use get_uint() for "acpi-pcihp-io*" properties
5890942 platform-bus: use get_uint() for "addr" property
16b2431 bcm2835_fb: use {get, set}_uint() for "vcram-size" and "vcram-base"
52a5c50 aspeed: use {set, get}_uint() for "ram-size" property
8ee5ed0 pcihp: use get_uint() for "bsel" property
ec9a2db pc-dimm: make "size" property uint64
5b65717 pc-dimm: use get_uint() for dimm properties
e05a02f isa: use get_uint() for "io-base"
74e0f4b qdev: use appropriate getter/setters type
ea4b431 apic-common: make "id" property a uint32
917ff10 qdev: avoid type casts between signed and unsigned
c803fcd qdev: wrap default property value in an union
0153836 qdev: rename DEFINE_PROP_DEFAULT() to DEFINE_PROP_SIGNED()
00b6346 object: use more specific property type names
6ebdb40 q35: fix get_mmcfg_size to use uint64 visitor
b66da0f object: add uint property setter/getter
3123cbf qapi: update the qobject visitor to use QNUM_U64
f65fbd4 json: learn to parse uint64 numbers
cbe9a26 qnum: add uint type
47d9e93 tests: remove /{qnum, qlist, dict}/destroy test
19e22e7 qapi: Remove visit_start_alternate() parameter promote_int
8564a30 qapi: merge QInt and QFloat in QNum
3a28906 qapi: minor refactoring
5c73880 tests: add more int/number ranges checks
98c8358 tests: Remove test cases for alternates of 'number' and 'int'
d2fce78 object: fix potential leak in getters
0c9c6c3 qdev: remove PropertyInfo.qtype field
=== OUTPUT BEGIN ===
Checking PATCH 1/43: qdev: remove PropertyInfo.qtype field...
Checking PATCH 2/43: object: fix potential leak in getters...
Checking PATCH 3/43: tests: Remove test cases for alternates of 'number' and
'int'...
Checking PATCH 4/43: tests: add more int/number ranges checks...
Checking PATCH 5/43: qapi: minor refactoring...
Checking PATCH 6/43: qapi: merge QInt and QFloat in QNum...
ERROR: do not use C99 // comments
#1971: FILE: tests/check-qnum.c:39:
+// destroy doesn't exit yet
ERROR: do not use C99 // comments
#1987: FILE: tests/check-qnum.c:55:
+// destroy doesn't exit yet
total: 2 errors, 0 warnings, 1894 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 7/43: qapi: Remove visit_start_alternate() parameter
promote_int...
Checking PATCH 8/43: tests: remove /{qnum, qlist, dict}/destroy test...
Checking PATCH 9/43: qnum: add uint type...
Checking PATCH 10/43: json: learn to parse uint64 numbers...
Checking PATCH 11/43: qapi: update the qobject visitor to use QNUM_U64...
Checking PATCH 12/43: object: add uint property setter/getter...
Checking PATCH 13/43: q35: fix get_mmcfg_size to use uint64 visitor...
Checking PATCH 14/43: object: use more specific property type names...
Checking PATCH 15/43: qdev: rename DEFINE_PROP_DEFAULT() to
DEFINE_PROP_SIGNED()...
Checking PATCH 16/43: qdev: wrap default property value in an union...
Checking PATCH 17/43: qdev: avoid type casts between signed and unsigned...
Checking PATCH 18/43: apic-common: make "id" property a uint32...
Checking PATCH 19/43: qdev: use appropriate getter/setters type...
Checking PATCH 20/43: isa: use get_uint() for "io-base"...
Checking PATCH 21/43: pc-dimm: use get_uint() for dimm properties...
Checking PATCH 22/43: pc-dimm: make "size"
Re: [Qemu-devel] [PATCH v2 3/4] nvdimm: add a boolean option "restrict"
On 06/07/17 16:27 +0100, Stefan Hajnoczi wrote:
> On Tue, Jun 06, 2017 at 03:22:28PM +0800, Haozhong Zhang wrote:
> > If a vNVDIMM device is not backed by a DAX device and its "restrict"
> > option is enabled, bit 3 of state flags in its region mapping
> > structure will be set, in order to notify the guest of the lack of
> > write persistence guarantee. Once this bit is set, the guest OS may
> > mark the vNVDIMM device as read-only.
> >
> > This option is disabled by default for backwards compatibility. It's
> > recommended to enable for the formal usage.
>
> Good idea. I think the following is cleaner:
>
> DEFINE_PROP_ON_OFF_AUTO("readonly") on the 'nvdimm' device. The
> following states are available:
>
> * 'on' - ACPI_NFIT_MEM_NOT_ARMED is set
> * 'off' - ACPI_NFIT_MEM_NOT_ARMED is clear
> * 'auto' - ACPI_NFIT_MEM_NOT_ARMED set if backend is not persistent
>
> This new property defaults to 'auto'. Machine types older than
> pc-i440fx-2.10 and pc-q35-2.10 default to 'on'.
Shouldn't it be 'off' on older machine types? The older machine types
and older QEMU never check the backend and never set ACPI_NFIT_MEM_NOT_ARMED.
Haozhong
Re: [Qemu-devel] [PATCH 6/7] spapr: Clean up handling of DR-indicator
On Wed, Jun 07, 2017 at 06:11:03PM -0500, Michael Roth wrote:
> Quoting David Gibson (2017-06-06 20:28:51)
> > On Tue, Jun 06, 2017 at 04:04:33PM -0500, Michael Roth wrote:
> > > Quoting David Gibson (2017-06-06 03:32:20)
> > > > There are 3 types of "indicator" associated with hotplug in the PAPR
> > > > spec
> > > > the "allocation state", "isolation state" and "DR-indicator". The first
> > > > two are intimately tied to the various state transitions associated with
> > > > hotplug. The DR-indicator, however, is different and simpler.
> > > >
> > > > It's basically just a guest controlled variable which can be used by the
> > > > guest to flag state or problems associated with a device. The idea is
> > > > that
> > > > the hypervisor can use it to present information back on management
> > > > consoles (on some machines with PowerVM it may even control physical
> > > > LEDs
> > > > on the machine case associated with the relevant device).
> > > >
> > > > For that reason, there's only ever likely to be a single update
> > > > implementation so the set_indicator_state method isn't useful. Replace
> > > > it
> > > > with a direct function call.
> > > >
> > > > While we're there, make some small associated cleanups:
> > > > * PAPR doesn't use the term "indicator state", just "DR-indicator" and
> > > > the allocation state and isolation state are also considered
> > > > "indicators".
> > > > Rename things to be less confusing
> > > > * Fold set_indicator_state() and rtas_set_indicator_state() into a
> > > > single
> > > > rtas_set_dr_indicator() function.
> > > >
> > > > Signed-off-by: David Gibson
> > > > ---
> > > > hw/ppc/spapr_drc.c | 25 -
> > > > hw/ppc/trace-events| 2 +-
> > > > include/hw/ppc/spapr_drc.h | 16
> > > > 3 files changed, 17 insertions(+), 26 deletions(-)
> > > >
> > > > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> > > > index 6c2fa93..a4ece2e 100644
> > > > --- a/hw/ppc/spapr_drc.c
> > > > +++ b/hw/ppc/spapr_drc.c
> > > > @@ -116,14 +116,6 @@ static uint32_t
> > > > set_isolation_state(sPAPRDRConnector *drc,
> > > > return RTAS_OUT_SUCCESS;
> > > > }
> > > >
> > > > -static uint32_t set_indicator_state(sPAPRDRConnector *drc,
> > > > -sPAPRDRIndicatorState state)
> > > > -{
> > > > -trace_spapr_drc_set_indicator_state(spapr_drc_index(drc), state);
> > > > -drc->indicator_state = state;
> > > > -return RTAS_OUT_SUCCESS;
> > > > -}
> > > > -
> > > > static uint32_t set_allocation_state(sPAPRDRConnector *drc,
> > > > sPAPRDRAllocationState state)
> > > > {
> > > > @@ -313,7 +305,7 @@ static void attach(sPAPRDRConnector *drc,
> > > > DeviceState *d, void *fdt,
> > > > if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_PCI) {
> > > > drc->isolation_state = SPAPR_DR_ISOLATION_STATE_UNISOLATED;
> > > > }
> > > > -drc->indicator_state = SPAPR_DR_INDICATOR_STATE_ACTIVE;
> > > > +drc->dr_indicator = SPAPR_DR_INDICATOR_ACTIVE;
> > > >
> > > > drc->dev = d;
> > > > drc->fdt = fdt;
> > > > @@ -386,7 +378,7 @@ static void detach(sPAPRDRConnector *drc,
> > > > DeviceState *d, Error **errp)
> > > > }
> > > > }
> > > >
> > > > -drc->indicator_state = SPAPR_DR_INDICATOR_STATE_INACTIVE;
> > > > +drc->dr_indicator = SPAPR_DR_INDICATOR_INACTIVE;
> > > >
> > > > /* Calling release callbacks based on spapr_drc_type(drc). */
> > > > switch (spapr_drc_type(drc)) {
> > > > @@ -499,7 +491,7 @@ static const VMStateDescription vmstate_spapr_drc =
> > > > {
> > > > .fields = (VMStateField []) {
> > > > VMSTATE_UINT32(isolation_state, sPAPRDRConnector),
> > > > VMSTATE_UINT32(allocation_state, sPAPRDRConnector),
> > > > -VMSTATE_UINT32(indicator_state, sPAPRDRConnector),
> > > > +VMSTATE_UINT32(dr_indicator, sPAPRDRConnector),
> > > > VMSTATE_BOOL(configured, sPAPRDRConnector),
> > > > VMSTATE_BOOL(awaiting_release, sPAPRDRConnector),
> > > > VMSTATE_BOOL(awaiting_allocation, sPAPRDRConnector),
> > > > @@ -614,7 +606,6 @@ static void
> > > > spapr_dr_connector_class_init(ObjectClass *k, void *data)
> > > > dk->realize = realize;
> > > > dk->unrealize = unrealize;
> > > > drck->set_isolation_state = set_isolation_state;
> > > > -drck->set_indicator_state = set_indicator_state;
> > > > drck->set_allocation_state = set_allocation_state;
> > > > drck->attach = attach;
> > > > drck->detach = detach;
> > > > @@ -895,17 +886,17 @@ static uint32_t
> > > > rtas_set_allocation_state(uint32_t idx, uint32_t state)
> > > > return drck->set_allocation_state(drc, state);
> > > > }
> > > >
> > > > -static uint32_t rtas_set_indicator_state(uint32_t idx, uint32_t state)
> > > > +static uint32_t rtas_set_dr_indicator(uint32_t idx, uint32_t state)
> > > > {
> > > > sPA
Re: [Qemu-devel] [RFC 2/3] spapr: Start hotplugged PCI devices in ISOLATED state
On Wed, Jun 07, 2017 at 05:49:06PM -0500, Michael Roth wrote: > Quoting David Gibson (2017-06-06 08:05:33) > > PCI DRCs, and only PCI DRCs, are immediately moved to UNISOLATED isolation > > state once the device is attached. This has been there from the initial > > implementation, and it's not clear why. > > > > The state diagram in PAPR 13.4 suggests PCI devices should start in > > ISOLATED state until the guest moves them into UNISOLATED, and the code in > > the guest-side drmgr tool seems to work that way too. > > I think this was a misguided attempt to disallow detach() to finalize a > device immediately after attach(), but up until v1.3.3 drmgr actually > explicitly put the device right back into ISOLATED before doing > entity-sense, then back to UNISOLATED right before calling > configure-connector and eventually bringing the device to a configured > state. So I doesn't seem like this would have had any effect up until > drmgr v1.3.3+. > > For drmgr v1.3.3+, this patch would have the effect of allowing detach() > during the initial entity-sense/set-power-level RTAS calls the guest > might be doing in response to attach(), but the guest code seems capable > of dealing with that particular case gracefully. Ah, ok. > I'm a bit concerned if we have *multiple* attach()/detach() pairs being > executed while drmgr is processing a single hotplug add event though: > > host guest > > attach() > notify >rtas-set-indicator(DR_INDICATOR_ON) >rtas-entity-sense => device_present >rtas-set-power-level(on) > detach() > attach() >rtas-set-sensor-state(ISOLATION_STATE_UNISOLATED) >rtas-configure-connector => configured >guest actually onlines device, but dr-indicator and >power domain aren't necessarily in the expected >state > > In practice, this one isn't a big issue since we emulate an > 'automatic' power domain in QEMU that makes any rtas-set-power-level > calls a no-op, and we don't rely on the dr-indicator (anymore, at > least), but it does end up being the wrong value, and makes me think > we should find some way to disallow immediate detach() after attach() > outside of the scenarios set_signalled() was added for. So I think > this patch seems reasonable, but maybe patch 3 should instead > repurpose set_signalled to handle this, or be replaced with some > other alternative that has the same effect. I see your point, but I don't think it's really worth worrying aboutg. For the DR-indicator, I don't particularly care about the state of a virtual LED in weird edge cases like this. From the guest POV it seems bogus to me to adjust the LED state before unisolating the device, but whatever. If we do ever implement explicit power control (unlikely) then the detach()/attach() would either not be permitted with device power on, or it would revert power to off, in which case the attempt to move to UNISOLATE would fail. -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
Re: [Qemu-devel] [PATCHv2 0/7] spapr: DRC cleanups (part III)
On Wed, Jun 07, 2017 at 06:22:16PM -0500, Michael Roth wrote: > Quoting David Gibson (2017-06-06 22:20:11) > > A third batch of cleanups to the DRC code. This continues to clear > > away relatively simple cruft, to get a clearer look at the fundamental > > state handling. > > > > Changes since v1: > > * Some comment updates suggested by Mike Roth > > * Changed approach to the get_name cleanup, using generated on the > > fly names, instead of externally assigned names > > * Added in some cleanups to hotplug code in spapr_pci.c > > Series: > > Reviewed-by: Michael Roth > Acked-by: Michael Roth Thanks. Patches merged into ppc-for-2.10. -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
Re: [Qemu-devel] [PATCH v2 2/4] nvdimm: warn if the backend is not a DAX device
On 06/07/17 16:14 +0100, Stefan Hajnoczi wrote:
> On Tue, Jun 06, 2017 at 03:22:27PM +0800, Haozhong Zhang wrote:
> > diff --git a/util/osdep.c b/util/osdep.c
> > index a2863c8e53..02881f96bc 100644
> > --- a/util/osdep.c
> > +++ b/util/osdep.c
> > @@ -471,3 +471,64 @@ writev(int fd, const struct iovec *iov, int iov_cnt)
> > return readv_writev(fd, iov, iov_cnt, true);
> > }
> > #endif
> > +
> > +#ifdef __linux__
> > +static ssize_t qemu_dev_dax_sysfs_read(int fd, const char *entry,
> > + char *buf, size_t len)
> > +{
> > +ssize_t read_bytes;
> > +struct stat st;
> > +unsigned int major, minor;
> > +char *path, *pos;
> > +int sysfs_fd;
> > +
> > +if (fstat(fd, &st)) {
> > +return 0;
> > +}
> > +
> > +major = major(st.st_rdev);
> > +minor = minor(st.st_rdev);
> > +path = g_strdup_printf("/sys/dev/char/%u:%u/%s", major, minor, entry);
> > +
> > +sysfs_fd = open(path, O_RDONLY);
> > +g_free(path);
> > +if (sysfs_fd == -1) {
> > +return 0;
> > +}
> > +
> > +read_bytes = read(sysfs_fd, buf, len - 1);
> > +close(sysfs_fd);
> > +if (read_bytes > 0) {
> > +buf[read_bytes] = '\0';
> > +pos = g_strstr_len(buf, read_bytes, "\n");
> > +if (pos) {
> > +*pos = '\0';
> > +}
>
> Should read_bytes be adjusted since we made the string shorter?
Yes, I'll change in the next version.
Thanks,
Haozhong
Re: [Qemu-devel] [PATCH v2 2/4] nvdimm: warn if the backend is not a DAX device
On 06/06/17 10:59 -0700, Dan Williams wrote:
> On Tue, Jun 6, 2017 at 12:22 AM, Haozhong Zhang
> wrote:
> > Applications in Linux guest that use device-dax never trigger flush
> > that can be trapped by KVM/QEMU. Meanwhile, if the host backend is not
> > device-dax, QEMU cannot guarantee the persistence of guest writes.
> > Before solving this flushing problem, QEMU should warn users if the
> > host backend is not device-dax.
> >
> > Signed-off-by: Haozhong Zhang
> > Message-id:
> > capcyv4hv2-zw8smcrtd0p_86kgr3dhovne+6t5sy2u7wxg3...@mail.gmail.com
> > ---
> > hw/mem/nvdimm.c | 6 ++
> > include/qemu/osdep.h | 9
> > util/osdep.c | 61
> >
> > 3 files changed, 76 insertions(+)
> >
> > diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
> > index a9b0863f20..b23542fbdf 100644
> > --- a/hw/mem/nvdimm.c
> > +++ b/hw/mem/nvdimm.c
> > @@ -26,6 +26,7 @@
> > #include "qapi/error.h"
> > #include "qapi/visitor.h"
> > #include "hw/mem/nvdimm.h"
> > +#include "qemu/error-report.h"
> >
> > static void nvdimm_get_label_size(Object *obj, Visitor *v, const char
> > *name,
> >void *opaque, Error **errp)
> > @@ -84,6 +85,11 @@ static void nvdimm_realize(PCDIMMDevice *dimm, Error
> > **errp)
> > NVDIMMDevice *nvdimm = NVDIMM(dimm);
> > uint64_t align, pmem_size, size = memory_region_size(mr);
> >
> > +if (!qemu_fd_is_dev_dax(memory_region_get_fd(mr))) {
> > +error_report("warning: nvdimm backend does not look like a DAX
> > device, "
> > + "unable to guarantee persistence of guest writes");
> > +}
> > +
> > align = memory_region_get_alignment(mr);
> >
> > pmem_size = size - nvdimm->label_size;
> > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> > index 1c9f5e260c..7f26af371e 100644
> > --- a/include/qemu/osdep.h
> > +++ b/include/qemu/osdep.h
> > @@ -470,4 +470,13 @@ char *qemu_get_pid_name(pid_t pid);
> > */
> > pid_t qemu_fork(Error **errp);
> >
> > +/**
> > + * qemu_fd_is_dev_dax:
> > + *
> > + * Check whether @fd describes a DAX device.
> > + *
> > + * Returns true if it is; otherwise, return false.
> > + */
> > +bool qemu_fd_is_dev_dax(int fd);
> > +
> > #endif
> > diff --git a/util/osdep.c b/util/osdep.c
> > index a2863c8e53..02881f96bc 100644
> > --- a/util/osdep.c
> > +++ b/util/osdep.c
> > @@ -471,3 +471,64 @@ writev(int fd, const struct iovec *iov, int iov_cnt)
> > return readv_writev(fd, iov, iov_cnt, true);
> > }
> > #endif
> > +
> > +#ifdef __linux__
> > +static ssize_t qemu_dev_dax_sysfs_read(int fd, const char *entry,
> > + char *buf, size_t len)
> > +{
> > +ssize_t read_bytes;
> > +struct stat st;
> > +unsigned int major, minor;
> > +char *path, *pos;
> > +int sysfs_fd;
> > +
> > +if (fstat(fd, &st)) {
> > +return 0;
> > +}
> > +
> > +major = major(st.st_rdev);
> > +minor = minor(st.st_rdev);
> > +path = g_strdup_printf("/sys/dev/char/%u:%u/%s", major, minor, entry);
> > +
> > +sysfs_fd = open(path, O_RDONLY);
> > +g_free(path);
> > +if (sysfs_fd == -1) {
> > +return 0;
> > +}
> > +
> > +read_bytes = read(sysfs_fd, buf, len - 1);
> > +close(sysfs_fd);
> > +if (read_bytes > 0) {
> > +buf[read_bytes] = '\0';
> > +pos = g_strstr_len(buf, read_bytes, "\n");
> > +if (pos) {
> > +*pos = '\0';
> > +}
> > +}
> > +
> > +return read_bytes;
> > +}
> > +#endif /* __linux__ */
> > +
> > +bool qemu_fd_is_dev_dax(int fd)
> > +{
> > +bool is_dax = false;
> > +
> > +#ifdef __linux__
> > +char devtype[7];
> > +ssize_t len;
> > +
> > +if (fd == -1) {
> > +return false;
> > +}
> > +
> > +len = qemu_dev_dax_sysfs_read(fd, "device/devtype",
> > + devtype, sizeof(devtype));
> > +if (len <= 0) {
> > +return false;
> > +}
> > +is_dax = !strncmp(devtype, "nd_dax", len);
>
> There's no guarantee that device-dax instances are always parented by
> an "nd_dax" device-type. A more reliable check is to see if
> "/sys/dev/char/%u:%u/subsystem" points to "/sys/class/dax".
Thanks for pointing out this, will change in the next version.
Haozhong
Re: [Qemu-devel] [Qemu-ppc] [PATCH] pseries: Correct panic behaviour for pseries machine type
On Wed, Jun 07, 2017 at 07:10:55PM +0200, Thomas Huth wrote:
> On 07.06.2017 16:34, Paolo Bonzini wrote:
> >
> >
> > On 07/06/2017 09:33, Thomas Huth wrote:
> >> On 07.06.2017 09:07, David Gibson wrote:
> >>> The pseries machine type doesn't usually use the 'pvpanic' device as such,
> >>> because it has a firmware/hypervisor facility with roughly the same
> >>> purpose. The 'ibm,os-term' RTAS call notifies the hypervisor that the
> >>> guest has crashed.
> >>>
> >>> Our implementation of this call was sending a GUEST_PANICKED qmp event;
> >>> however, it was not doing the other usual panic actions, making its
> >>> behaviour different from pvpanic for no good reason.
> >>>
> >>> To correct this, we should call qemu_system_guest_panicked() rather than
> >>> directly sending the panic event.
> >>>
> >>> Signed-off-by: David Gibson
> >>> ---
> >>> hw/ppc/spapr_rtas.c | 7 ++-
> >>> 1 file changed, 2 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> >>> index 707c4d4..94a2799 100644
> >>> --- a/hw/ppc/spapr_rtas.c
> >>> +++ b/hw/ppc/spapr_rtas.c
> >>> @@ -293,12 +293,9 @@ static void rtas_ibm_os_term(PowerPCCPU *cpu,
> >>> target_ulong args,
> >>> uint32_t nret, target_ulong rets)
> >>> {
> >>> -target_ulong ret = 0;
> >>> +qemu_system_guest_panicked(NULL);
> >>>
> >>> -qapi_event_send_guest_panicked(GUEST_PANIC_ACTION_PAUSE, false, NULL,
> >>> - &error_abort);
> >>> -
> >>> -rtas_st(rets, 0, ret);
> >>> +rtas_st(rets, 0, RTAS_OUT_SUCCESS);
> >>> }
> >>>
> >>> static void rtas_set_power_level(PowerPCCPU *cpu, sPAPRMachineState
> >>> *spapr,
> >>>
> >>
> >> If I get that qemu_system_guest_panicked() function right, it will stop
> >> the VM, won't it? That contradicts the LoPAPR spec that says that the
> >> RTAS call returns if the "ibm,extended-os-term" property is available in
> >> the device tree.
> >
> > It does return... but only after the user starts the guest again with
> > "cont".
>
> OK, I guess that's enough to say that the "ibm,extended-os-term"
> property can stay ... so I think the patch is fine as it is right now.
So.. can I have an R-b?
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [Qemu-devel] [PATCH] vhost-user-bridge: fix iov_restore_front() warning
On Wed, Jun 07, 2017 at 07:00:46PM +, Marc-André Lureau wrote:
> Hi, Michael, could you pick this patch?
>
> thanks
OK. Pls cc me on patches you want me to review.
> On Fri, Jun 2, 2017 at 12:26 PM Marc-André Lureau <
> marcandre.lur...@redhat.com> wrote:
>
> > CC tests/vhost-user-bridge.o
> > /home/dgilbert/git/qemu-world3/tests/vhost-user-bridge.c:228:23: warning:
> > variables 'front' and 'iov' used in loop condition not modified in loop
> > body [-Wfor-loop-analysis]
> > for (cur = front; front != iov; cur++) {
> > ^~~~
> > 1 warning generated.
> >
> > Fix the loop, document the function, and fix some related assert().
> >
> > In practice, the loop bug was harmless because the front sg buffer is
> > enough to discard/restore the header size.
> >
> > Reported-by: Dr. David Alan Gilbert
> > Signed-off-by: Marc-André Lureau
> > ---
> > tests/vhost-user-bridge.c | 11 +--
> > 1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/tests/vhost-user-bridge.c b/tests/vhost-user-bridge.c
> > index 8618c20d53..1e5b5ca3da 100644
> > --- a/tests/vhost-user-bridge.c
> > +++ b/tests/vhost-user-bridge.c
> > @@ -220,12 +220,18 @@ vubr_handle_tx(VuDev *dev, int qidx)
> > free(elem);
> > }
> >
> > +
> > +/* this function reverse the effect of iov_discard_front() it must be
> > + * called with 'front' being the original struct iovec and 'bytes'
> > + * being the number of bytes you shaved off
> > + */
> > static void
> > iov_restore_front(struct iovec *front, struct iovec *iov, size_t bytes)
> > {
> > struct iovec *cur;
> >
> > -for (cur = front; front != iov; cur++) {
> > +for (cur = front; cur != iov; cur++) {
> > +assert(bytes >= cur->iov_len);
> > bytes -= cur->iov_len;
> > }
> >
> > @@ -302,7 +308,8 @@ vubr_backend_recv_cb(int sock, void *ctx)
> > }
> > iov_from_buf(sg, elem->in_num, 0, &hdr, sizeof hdr);
> > total += hdrlen;
> > -assert(iov_discard_front(&sg, &num, hdrlen) == hdrlen);
> > +ret = iov_discard_front(&sg, &num, hdrlen);
> > +assert(ret == hdrlen);
> > }
> >
> > struct msghdr msg = {
> > --
> > 2.13.0.91.g00982b8dd
> >
> >
> > --
> Marc-André Lureau
Re: [Qemu-devel] [PULL 18/33] vhost-user-scsi: Introduce vhost-user-scsi host device
On Wed, Jun 07, 2017 at 02:56:57PM -0400, Paolo Bonzini wrote: > > > > This could be documented, > > > > It is documented AFAIK. Pls take a look at the spec documentation. > > Found it now. It's not under GET_VRING_BASE, it's under "starting > and stopping rings"---fair enough. > > In the case of vhost-user-scsi, however, QEMU also must not proceed > until vhost-user-scsi has drained the pending I/O---and this pending > I/O would be completed _after_ QEMU has sent GET_VRING_BASE. Weird. Doesn't QEMU wait for response to GET_VRING_BASE? I think it does since we migrate the returned value. Spec says: Client must only process each ring when it is started. so this isn't expected. I guess whoever wrote vhost-user-scsi understood "process" as "start processing". What was intended is reading or writing any part of ring. The used ring must not be updated after ring is stopped. A spec clarification might in order. > Is this handled by VHOST_USER_PROTOCOL_F_REPLY_ACK already? If so, > migration would be denied if the server lacks that protocol feature. > > Paolo GET_VRING_BASE does not need an ack, after respoding ring should be stopped. > > > but perhaps it's best to add a START_STOP > > > feature and message to the vhost-user protocol? > > > > We just never need to GET_VRING_BASE if ring keeps going - > > makes no sense since base gets invalidated immediately. > > > > > > > > > The feature then can be optional for vhost-user-net and mandatory for > > > vhost-user-scsi. When this is done we can remove .unmigratable. > > > > > > Thanks, > > > > > > Paolo > > > > If vhost-user-scsi does not stop the ring after responding to > > GET_VRING_BASE, it's just a bug that needs to be fixed. > > > > > > > > > > > > Can you please send a version of your patch that uses .unmigratable? > > > > > > > > Sure I can do that. We can work on the migration later on. > > > > > > > > > > > > > > I'll send a v6 that momentarily drops vhost-scsi, but I intend to > > > > > include it again in the next pull request. > > > > > > > > Sounds good to me. > > > > > > > > Felipe > > > > > > > > > > > > > > Thanks, > > > > > > > > > > Paolo > > > > > > > > > >
Re: [Qemu-devel] [PATCH v4 0/7] tcg: allocate TB structs preceding translate
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 20170607155536.1193-1-...@twiddle.net
Subject: [Qemu-devel] [PATCH v4 0/7] tcg: allocate TB structs preceding
translate
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
git config --local diff.renamelimit 0
git config --local diff.renames True
commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
a5f5544 target/arm: Use ldr (literal) for goto_tb
15a92d8 target/arm: Try pc-relative addresses for movi
c270c42 target/arm: Remove limit on code buffer size
92df92d target/arm: Use indirect branch for goto_tb
ca5d3fd tcg/aarch64: Use ADR in tcg_out_movi
c62a640 tcg: allocate TB structs before the corresponding translated code
b3e2a1f util: add cacheinfo
=== OUTPUT BEGIN ===
Checking PATCH 1/7: util: add cacheinfo...
ERROR: do not initialise globals to 0 or NULL
#149: FILE: util/cacheinfo.c:11:
+int qemu_icache_linesize = 0;
ERROR: do not initialise globals to 0 or NULL
#150: FILE: util/cacheinfo.c:12:
+int qemu_dcache_linesize = 0;
ERROR: space prohibited after that '&&' (ctx:ExW)
#187: FILE: util/cacheinfo.c:49:
+&& buf[i].Cache.Level == 1) {
^
WARNING: architecture specific defines should be avoided
#209: FILE: util/cacheinfo.c:71:
+# if defined(__APPLE__)
total: 3 errors, 1 warnings, 207 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 2/7: tcg: allocate TB structs before the corresponding
translated code...
Checking PATCH 3/7: tcg/aarch64: Use ADR in tcg_out_movi...
Checking PATCH 4/7: target/arm: Use indirect branch for goto_tb...
Checking PATCH 5/7: target/arm: Remove limit on code buffer size...
Checking PATCH 6/7: target/arm: Try pc-relative addresses for movi...
ERROR: code indent should never use tabs
#54: FILE: tcg/arm/tcg-target.inc.c:446:
+^I}$
ERROR: code indent should never use tabs
#64: FILE: tcg/arm/tcg-target.inc.c:453:
+^I}$
total: 2 errors, 0 warnings, 54 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 7/7: target/arm: Use ldr (literal) for goto_tb...
=== OUTPUT END ===
Test command exited with code: 1
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-de...@freelists.org
Re: [Qemu-devel] [PATCH v1 1/1] char-socket: Don't report TCP socket waiting as an error
On Wed, Jun 7, 2017 at 12:19 AM, Markus Armbruster wrote: > Paolo Bonzini writes: > >> On 06/06/2017 18:30, Alistair Francis wrote: This is somehow confusing. I don't think it is worth having another qemu_log_stderr() function rather than using error_report() but this very call might deserve a comment explaining this unusual use. What do you think? >>> >>> The problem with stderr is that this isn't an error. Some uses of QEMU >>> (inside Eclipse for example) flag everything printed on stderr as red >>> which confuses users that they are seeing an error when they really >>> aren't. >> >> But they are wrong. > > Concur. We also print warnings and informational messages to stderr. > > We should make errors easy to recognize. Fortunately, error_report() > prints errors to stderr in a rigid format. Unfortunately, error > messages bypassing error_report() still exist in places. We suck. > > The format is > > timestamp-if-enabled progname ':' location message > > timestamp-if-enabled is normally empty. With -msg timestamp=on, it's > the current time in ISO 8601 format, followed by a space. > > progname is the program name (main()'s argv[0]). > > location is either empty, or a reference to the command line or a > configuration file. > > See error_vreport() for details. Ok, but this isn't an error, it's more information. So it sounds like we should still print to stderr but not print in the format described above? Thanks, Alistair > > [...] >
Re: [Qemu-devel] [RFC 2/3] spapr: Start hotplugged PCI devices in ISOLATED state
Quoting Michael Roth (2017-06-07 17:49:06)
> Quoting David Gibson (2017-06-06 08:05:33)
> > PCI DRCs, and only PCI DRCs, are immediately moved to UNISOLATED isolation
> > state once the device is attached. This has been there from the initial
> > implementation, and it's not clear why.
> >
> > The state diagram in PAPR 13.4 suggests PCI devices should start in
> > ISOLATED state until the guest moves them into UNISOLATED, and the code in
> > the guest-side drmgr tool seems to work that way too.
>
> I think this was a misguided attempt to disallow detach() to finalize a
> device immediately after attach(), but up until v1.3.3 drmgr actually
> explicitly put the device right back into ISOLATED before doing
> entity-sense, then back to UNISOLATED right before calling
> configure-connector and eventually bringing the device to a configured
> state. So I doesn't seem like this would have had any effect up until
> drmgr v1.3.3+.
>
> For drmgr v1.3.3+, this patch would have the effect of allowing detach()
> during the initial entity-sense/set-power-level RTAS calls the guest
> might be doing in response to attach(), but the guest code seems capable
> of dealing with that particular case gracefully.
>
> I'm a bit concerned if we have *multiple* attach()/detach() pairs being
> executed while drmgr is processing a single hotplug add event though:
>
> host guest
>
> attach()
> notify
>rtas-set-indicator(DR_INDICATOR_ON)
>rtas-entity-sense => device_present
>rtas-set-power-level(on)
> detach()
> attach()
>rtas-set-sensor-state(ISOLATION_STATE_UNISOLATED)
>rtas-configure-connector => configured
>guest actually onlines device, but dr-indicator and
>power domain aren't necessarily in the expected
>state
>
> In practice, this one isn't a big issue since we emulate an
> 'automatic' power domain in QEMU that makes any rtas-set-power-level
> calls a no-op, and we don't rely on the dr-indicator (anymore, at
> least), but it does end up being the wrong value, and makes me think
Actually, since we set it explicitly in attach(), dr-indicator does end
up being correct, but correct for the wrong reasons still.
> we should find some way to disallow immediate detach() after attach()
> outside of the scenarios set_signalled() was added for. So I think
> this patch seems reasonable, but maybe patch 3 should instead
> repurpose set_signalled to handle this, or be replaced with some
> other alternative that has the same effect.
>
> >
> > Signed-off-by: David Gibson
>
> Reviewed-by: Michael Roth
>
> > ---
> > hw/ppc/spapr_drc.c | 10 --
> > 1 file changed, 10 deletions(-)
> >
> > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> > index c73fae0..22f9224 100644
> > --- a/hw/ppc/spapr_drc.c
> > +++ b/hw/ppc/spapr_drc.c
> > @@ -291,16 +291,6 @@ void spapr_drc_attach(sPAPRDRConnector *drc,
> > DeviceState *d, void *fdt,
> > }
> > g_assert(fdt || coldplug);
> >
> > -/* NOTE: setting initial isolation state to UNISOLATED means we can't
> > - * detach unless guest has a userspace/kernel that moves this state
> > - * back to ISOLATED in response to an unplug event, or this is done
> > - * manually by the admin prior. if we force things while the guest
> > - * may be accessing the device, we can easily crash the guest, so we
> > - * we defer completion of removal in such cases to the reset() hook.
> > - */
> > -if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_PCI) {
> > -drc->isolation_state = SPAPR_DR_ISOLATION_STATE_UNISOLATED;
> > -}
> > drc->dr_indicator = SPAPR_DR_INDICATOR_ACTIVE;
> >
> > drc->dev = d;
> > --
> > 2.9.4
> >
>
>
Re: [Qemu-devel] [PATCH v4 0/7] tcg: allocate TB structs preceding translate
On Wed, Jun 07, 2017 at 08:55:29 -0700, Richard Henderson wrote: > Then I've a few follow-up patches to take advantage of the new TB > placement for arm platforms. I've had a look at the asm output for > ppc64 and s390x, and don't see anything obvious that can be improved. Nice! Just tested patches 3-7 with an arm guest image on x86 and aarch64 hosts: Tested-by: Emilio G. Cota Thanks, E.
Re: [Qemu-devel] [PATCHv2 0/7] spapr: DRC cleanups (part III)
Quoting David Gibson (2017-06-06 22:20:11)
> A third batch of cleanups to the DRC code. This continues to clear
> away relatively simple cruft, to get a clearer look at the fundamental
> state handling.
>
> Changes since v1:
> * Some comment updates suggested by Mike Roth
> * Changed approach to the get_name cleanup, using generated on the
> fly names, instead of externally assigned names
> * Added in some cleanups to hotplug code in spapr_pci.c
Series:
Reviewed-by: Michael Roth
Acked-by: Michael Roth
>
> David Gibson (7):
> spapr: Clean up DR entity sense handling
> spapr: Don't misuse DR-indicator in spapr_recover_pending_dimm_state()
> spapr: Clean up RTAS set-indicator
> spapr: Clean up handling of DR-indicator
> spapr: Change DRC attach & detach methods to functions
> spapr: Fold spapr_phb_{add,remove}_pci_device() into their only
> callers
> spapr: Rework DRC name handling
>
> hw/ppc/spapr.c | 30 ++---
> hw/ppc/spapr_drc.c | 269
> -
> hw/ppc/spapr_pci.c | 72 +---
> hw/ppc/trace-events| 5 +-
> include/hw/ppc/spapr_drc.h | 30 ++---
> 5 files changed, 171 insertions(+), 235 deletions(-)
>
> --
> 2.9.4
>
Re: [Qemu-devel] [PATCH 6/7] spapr: Clean up handling of DR-indicator
Quoting David Gibson (2017-06-06 20:28:51)
> On Tue, Jun 06, 2017 at 04:04:33PM -0500, Michael Roth wrote:
> > Quoting David Gibson (2017-06-06 03:32:20)
> > > There are 3 types of "indicator" associated with hotplug in the PAPR spec
> > > the "allocation state", "isolation state" and "DR-indicator". The first
> > > two are intimately tied to the various state transitions associated with
> > > hotplug. The DR-indicator, however, is different and simpler.
> > >
> > > It's basically just a guest controlled variable which can be used by the
> > > guest to flag state or problems associated with a device. The idea is
> > > that
> > > the hypervisor can use it to present information back on management
> > > consoles (on some machines with PowerVM it may even control physical LEDs
> > > on the machine case associated with the relevant device).
> > >
> > > For that reason, there's only ever likely to be a single update
> > > implementation so the set_indicator_state method isn't useful. Replace it
> > > with a direct function call.
> > >
> > > While we're there, make some small associated cleanups:
> > > * PAPR doesn't use the term "indicator state", just "DR-indicator" and
> > > the allocation state and isolation state are also considered "indicators".
> > > Rename things to be less confusing
> > > * Fold set_indicator_state() and rtas_set_indicator_state() into a
> > > single
> > > rtas_set_dr_indicator() function.
> > >
> > > Signed-off-by: David Gibson
> > > ---
> > > hw/ppc/spapr_drc.c | 25 -
> > > hw/ppc/trace-events| 2 +-
> > > include/hw/ppc/spapr_drc.h | 16
> > > 3 files changed, 17 insertions(+), 26 deletions(-)
> > >
> > > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> > > index 6c2fa93..a4ece2e 100644
> > > --- a/hw/ppc/spapr_drc.c
> > > +++ b/hw/ppc/spapr_drc.c
> > > @@ -116,14 +116,6 @@ static uint32_t set_isolation_state(sPAPRDRConnector
> > > *drc,
> > > return RTAS_OUT_SUCCESS;
> > > }
> > >
> > > -static uint32_t set_indicator_state(sPAPRDRConnector *drc,
> > > -sPAPRDRIndicatorState state)
> > > -{
> > > -trace_spapr_drc_set_indicator_state(spapr_drc_index(drc), state);
> > > -drc->indicator_state = state;
> > > -return RTAS_OUT_SUCCESS;
> > > -}
> > > -
> > > static uint32_t set_allocation_state(sPAPRDRConnector *drc,
> > > sPAPRDRAllocationState state)
> > > {
> > > @@ -313,7 +305,7 @@ static void attach(sPAPRDRConnector *drc, DeviceState
> > > *d, void *fdt,
> > > if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_PCI) {
> > > drc->isolation_state = SPAPR_DR_ISOLATION_STATE_UNISOLATED;
> > > }
> > > -drc->indicator_state = SPAPR_DR_INDICATOR_STATE_ACTIVE;
> > > +drc->dr_indicator = SPAPR_DR_INDICATOR_ACTIVE;
> > >
> > > drc->dev = d;
> > > drc->fdt = fdt;
> > > @@ -386,7 +378,7 @@ static void detach(sPAPRDRConnector *drc, DeviceState
> > > *d, Error **errp)
> > > }
> > > }
> > >
> > > -drc->indicator_state = SPAPR_DR_INDICATOR_STATE_INACTIVE;
> > > +drc->dr_indicator = SPAPR_DR_INDICATOR_INACTIVE;
> > >
> > > /* Calling release callbacks based on spapr_drc_type(drc). */
> > > switch (spapr_drc_type(drc)) {
> > > @@ -499,7 +491,7 @@ static const VMStateDescription vmstate_spapr_drc = {
> > > .fields = (VMStateField []) {
> > > VMSTATE_UINT32(isolation_state, sPAPRDRConnector),
> > > VMSTATE_UINT32(allocation_state, sPAPRDRConnector),
> > > -VMSTATE_UINT32(indicator_state, sPAPRDRConnector),
> > > +VMSTATE_UINT32(dr_indicator, sPAPRDRConnector),
> > > VMSTATE_BOOL(configured, sPAPRDRConnector),
> > > VMSTATE_BOOL(awaiting_release, sPAPRDRConnector),
> > > VMSTATE_BOOL(awaiting_allocation, sPAPRDRConnector),
> > > @@ -614,7 +606,6 @@ static void spapr_dr_connector_class_init(ObjectClass
> > > *k, void *data)
> > > dk->realize = realize;
> > > dk->unrealize = unrealize;
> > > drck->set_isolation_state = set_isolation_state;
> > > -drck->set_indicator_state = set_indicator_state;
> > > drck->set_allocation_state = set_allocation_state;
> > > drck->attach = attach;
> > > drck->detach = detach;
> > > @@ -895,17 +886,17 @@ static uint32_t rtas_set_allocation_state(uint32_t
> > > idx, uint32_t state)
> > > return drck->set_allocation_state(drc, state);
> > > }
> > >
> > > -static uint32_t rtas_set_indicator_state(uint32_t idx, uint32_t state)
> > > +static uint32_t rtas_set_dr_indicator(uint32_t idx, uint32_t state)
> > > {
> > > sPAPRDRConnector *drc = spapr_drc_by_index(idx);
> > > -sPAPRDRConnectorClass *drck;
> > >
> > > if (!drc) {
> > > return RTAS_OUT_PARAM_ERROR;
> > > }
> > >
> > > -drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> > > -return drck->set_indicator_state(drc, state);
> > > +trace_sp
Re: [Qemu-devel] [RFC 1/3] spapr: Fold spapr_phb_add_pci_device() into its only caller
Quoting David Gibson (2017-06-06 20:33:07)
> On Tue, Jun 06, 2017 at 04:37:27PM -0500, Michael Roth wrote:
> > Quoting David Gibson (2017-06-06 08:05:32)
> > > This function is fairly short, and so is its only caller. There's no
> > > particular logical distinction between them, so fold them together.
> > >
> > > Signed-off-by: David Gibson
> > > ---
> > > hw/ppc/spapr_pci.c | 53
> > > ++---
> > > 1 file changed, 22 insertions(+), 31 deletions(-)
> > >
> > > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> > > index 46e736d..a216f61 100644
> > > --- a/hw/ppc/spapr_pci.c
> > > +++ b/hw/ppc/spapr_pci.c
> > > @@ -1344,30 +1344,6 @@ static int spapr_create_pci_child_dt(sPAPRPHBState
> > > *phb, PCIDevice *dev,
> > > return offset;
> > > }
> > >
> > > -static void spapr_phb_add_pci_device(sPAPRDRConnector *drc,
> > > - sPAPRPHBState *phb,
> > > - PCIDevice *pdev,
> > > - Error **errp)
> > > -{
> > > -DeviceState *dev = DEVICE(pdev);
> > > -void *fdt = NULL;
> > > -int fdt_start_offset = 0, fdt_size;
> > > -
> > > -fdt = create_device_tree(&fdt_size);
> > > -fdt_start_offset = spapr_create_pci_child_dt(phb, pdev, fdt, 0);
> > > -if (!fdt_start_offset) {
> > > -error_setg(errp, "Failed to create pci child device tree node");
> > > -goto out;
> > > -}
> > > -
> > > -spapr_drc_attach(drc, DEVICE(pdev),
> > > - fdt, fdt_start_offset, !dev->hotplugged, errp);
> > > -out:
> > > -if (*errp) {
> > > -g_free(fdt);
> > > -}
> > > -}
> > > -
> > > /* Callback to be called during DRC release. */
> > > void spapr_phb_remove_pci_device_cb(DeviceState *dev)
> > > {
> > > @@ -1429,6 +1405,8 @@ static void spapr_phb_hot_plug_child(HotplugHandler
> > > *plug_handler,
> > > Error *local_err = NULL;
> > > PCIBus *bus = PCI_BUS(qdev_get_parent_bus(DEVICE(pdev)));
> > > uint32_t slotnr = PCI_SLOT(pdev->devfn);
> > > +void *fdt = NULL;
> > > +int fdt_start_offset, fdt_size;
> > >
> > > /* if DR is disabled we don't need to do anything in the case of
> > > * hotplug or coldplug callbacks
> > > @@ -1438,10 +1416,10 @@ static void
> > > spapr_phb_hot_plug_child(HotplugHandler *plug_handler,
> > > * we need to let them know it's not enabled
> > > */
> > > if (plugged_dev->hotplugged) {
> > > -error_setg(errp, QERR_BUS_NO_HOTPLUG,
> > > +error_setg(&local_err, QERR_BUS_NO_HOTPLUG,
> > > object_get_typename(OBJECT(phb)));
> > > }
> > > -return;
> > > +goto out;
> > > }
> > >
> > > g_assert(drc);
> > > @@ -1452,16 +1430,23 @@ static void
> > > spapr_phb_hot_plug_child(HotplugHandler *plug_handler,
> > > */
> > > if (plugged_dev->hotplugged && bus->devices[PCI_DEVFN(slotnr, 0)] &&
> > > PCI_FUNC(pdev->devfn) != 0) {
> > > -error_setg(errp, "PCI: slot %d function 0 already ocuppied by
> > > %s,"
> > > +error_setg(&local_err, "PCI: slot %d function 0 already ocuppied
> > > by %s,"
> > > " additional functions can no longer be exposed to
> > > guest.",
> > > slotnr, bus->devices[PCI_DEVFN(slotnr, 0)]->name);
> > > -return;
> > > +goto out;
> > > }
> > >
> > > -spapr_phb_add_pci_device(drc, phb, pdev, &local_err);
> >
> > Since we never used local_err outside propagating it and immediately
> > bailing, and since we bail on errp in all prior callers, maybe we
> > should just drop local_err completely in favor errp.
>
> That doesn't quite work. The reason for the local_err pattern is so
> that we can tell locally if the error was triggered (errp might be
> NULL, so checking *errp isn't safe).
Ah, of course, not sure how I keep forgetting that detail.
>
> >
> > > +fdt = create_device_tree(&fdt_size);
> > > +fdt_start_offset = spapr_create_pci_child_dt(phb, pdev, fdt, 0);
> > > +if (!fdt_start_offset) {
> > > +error_setg(&local_err, "Failed to create pci child device tree
> > > node");
> > > +goto out;
> > > +}
> > > +
> > > +spapr_drc_attach(drc, DEVICE(pdev), fdt, fdt_start_offset,
> > > + !plugged_dev->hotplugged, &local_err);
> > > if (local_err) {
> > > -error_propagate(errp, local_err);
> > > -return;
> > > +goto out;
> > > }
> > >
> > > /* If this is function 0, signal hotplug for all the device
> > > functions.
> > > @@ -1485,6 +1470,12 @@ static void
> > > spapr_phb_hot_plug_child(HotplugHandler *plug_handler,
> > > }
> > > }
> > > }
> > > +
> > > +out:
> > > +if (local_err) {
> > > +error_propagate(errp, local_err);
> > > +g_free(fdt);
> > > +}
> > > }
> > >
> > > static vo
Re: [Qemu-devel] [RFC 2/3] spapr: Start hotplugged PCI devices in ISOLATED state
Quoting David Gibson (2017-06-06 08:05:33)
> PCI DRCs, and only PCI DRCs, are immediately moved to UNISOLATED isolation
> state once the device is attached. This has been there from the initial
> implementation, and it's not clear why.
>
> The state diagram in PAPR 13.4 suggests PCI devices should start in
> ISOLATED state until the guest moves them into UNISOLATED, and the code in
> the guest-side drmgr tool seems to work that way too.
I think this was a misguided attempt to disallow detach() to finalize a
device immediately after attach(), but up until v1.3.3 drmgr actually
explicitly put the device right back into ISOLATED before doing
entity-sense, then back to UNISOLATED right before calling
configure-connector and eventually bringing the device to a configured
state. So I doesn't seem like this would have had any effect up until
drmgr v1.3.3+.
For drmgr v1.3.3+, this patch would have the effect of allowing detach()
during the initial entity-sense/set-power-level RTAS calls the guest
might be doing in response to attach(), but the guest code seems capable
of dealing with that particular case gracefully.
I'm a bit concerned if we have *multiple* attach()/detach() pairs being
executed while drmgr is processing a single hotplug add event though:
host guest
attach()
notify
rtas-set-indicator(DR_INDICATOR_ON)
rtas-entity-sense => device_present
rtas-set-power-level(on)
detach()
attach()
rtas-set-sensor-state(ISOLATION_STATE_UNISOLATED)
rtas-configure-connector => configured
guest actually onlines device, but dr-indicator and
power domain aren't necessarily in the expected
state
In practice, this one isn't a big issue since we emulate an
'automatic' power domain in QEMU that makes any rtas-set-power-level
calls a no-op, and we don't rely on the dr-indicator (anymore, at
least), but it does end up being the wrong value, and makes me think
we should find some way to disallow immediate detach() after attach()
outside of the scenarios set_signalled() was added for. So I think
this patch seems reasonable, but maybe patch 3 should instead
repurpose set_signalled to handle this, or be replaced with some
other alternative that has the same effect.
>
> Signed-off-by: David Gibson
Reviewed-by: Michael Roth
> ---
> hw/ppc/spapr_drc.c | 10 --
> 1 file changed, 10 deletions(-)
>
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index c73fae0..22f9224 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -291,16 +291,6 @@ void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState
> *d, void *fdt,
> }
> g_assert(fdt || coldplug);
>
> -/* NOTE: setting initial isolation state to UNISOLATED means we can't
> - * detach unless guest has a userspace/kernel that moves this state
> - * back to ISOLATED in response to an unplug event, or this is done
> - * manually by the admin prior. if we force things while the guest
> - * may be accessing the device, we can easily crash the guest, so we
> - * we defer completion of removal in such cases to the reset() hook.
> - */
> -if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_PCI) {
> -drc->isolation_state = SPAPR_DR_ISOLATION_STATE_UNISOLATED;
> -}
> drc->dr_indicator = SPAPR_DR_INDICATOR_ACTIVE;
>
> drc->dev = d;
> --
> 2.9.4
>
[Qemu-devel] [RFC PATCH 1/3] tcg/aarch64: Introduce and use jump to register
Signed-off-by: Pranith Kumar
---
tcg/aarch64/tcg-target.inc.c | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/tcg/aarch64/tcg-target.inc.c b/tcg/aarch64/tcg-target.inc.c
index 1fa3bccc89..ab0a8caa03 100644
--- a/tcg/aarch64/tcg-target.inc.c
+++ b/tcg/aarch64/tcg-target.inc.c
@@ -819,6 +819,12 @@ static inline void tcg_out_goto(TCGContext *s,
tcg_insn_unit *target)
tcg_out_insn(s, 3206, B, offset);
}
+static inline void tcg_out_goto_register(TCGContext *s, intptr_t target)
+{
+tcg_out_movi(s, TCG_TYPE_I64, TCG_REG_TMP, target);
+tcg_out_insn(s, 3207, BR, TCG_REG_TMP);
+}
+
static inline void tcg_out_goto_noaddr(TCGContext *s)
{
/* We pay attention here to not modify the branch target by reading from
@@ -1364,10 +1370,10 @@ static void tcg_out_op(TCGContext *s, TCGOpcode opc,
case INDEX_op_exit_tb:
/* Reuse the zeroing that exists for goto_ptr. */
if (a0 == 0) {
-tcg_out_goto(s, s->code_gen_epilogue);
+tcg_out_goto_register(s, (intptr_t)(s->code_gen_epilogue));
} else {
tcg_out_movi(s, TCG_TYPE_I64, TCG_REG_X0, a0);
-tcg_out_goto(s, tb_ret_addr);
+tcg_out_goto_register(s, (intptr_t)(tb_ret_addr));
}
break;
--
2.13.0
[Qemu-devel] [RFC PATCH 0/3] Remove code buffer size limitation on aarch64 hosts
Hi, The following patches apply on top of tcg-next of rth's branch. These patches make use of LDR (literal) on aarch64 and enable us to remove the 128MB code buffer size limitation. Pranith Kumar (3): tcg/aarch64: Introduce and use jump to register tcg/aarch64: Introdue LDR (literal) generation for aarch64 tcg/aarch64: Remove code buffer size limitation include/exec/exec-all.h | 6 +- tcg/aarch64/tcg-target.inc.c | 42 +- translate-all.c | 2 -- 3 files changed, 22 insertions(+), 28 deletions(-) -- 2.13.0
[Qemu-devel] [RFC PATCH 2/3] tcg/aarch64: Introdue LDR (literal) for aarch64
Signed-off-by: Pranith Kumar
---
tcg/aarch64/tcg-target.inc.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/tcg/aarch64/tcg-target.inc.c b/tcg/aarch64/tcg-target.inc.c
index ab0a8caa03..e488aacadb 100644
--- a/tcg/aarch64/tcg-target.inc.c
+++ b/tcg/aarch64/tcg-target.inc.c
@@ -269,6 +269,8 @@ typedef enum {
I3207_BLR = 0xd63f,
I3207_RET = 0xd65f,
+/* Load literal for loading the address at pc-relative offset */
+I3305_LDR = 0x5800,
/* Load/store register. Described here as 3.3.12, but the helper
that emits them can transform to 3.3.10 or 3.3.13. */
I3312_STRB = 0x3800 | LDST_ST << 22 | MO_8 << 30,
@@ -388,6 +390,11 @@ static inline uint32_t tcg_in32(TCGContext *s)
#define tcg_out_insn(S, FMT, OP, ...) \
glue(tcg_out_insn_,FMT)(S, glue(glue(glue(I,FMT),_),OP), ## __VA_ARGS__)
+static void tcg_out_insn_3305(TCGContext *s, AArch64Insn insn, int imm19,
TCGReg rt)
+{
+tcg_out32(s, insn | (imm19 & 0x7) << 5 | rt);
+}
+
static void tcg_out_insn_3201(TCGContext *s, AArch64Insn insn, TCGType ext,
TCGReg rt, int imm19)
{
--
2.13.0
[Qemu-devel] [RFC PATCH 3/3] tcg/aarch64: Remove code buffer size limitation
This enables indirect jump on aarch64 hosts. Tested by booting an x86 guest on
aarch64 host.
Signed-off-by: Pranith Kumar
---
include/exec/exec-all.h | 6 +-
tcg/aarch64/tcg-target.inc.c | 25 ++---
translate-all.c | 2 --
3 files changed, 7 insertions(+), 26 deletions(-)
diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index 724ec73dce..a6bd3c7d1e 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -301,9 +301,8 @@ static inline void
tlb_flush_by_mmuidx_all_cpus_synced(CPUState *cpu,
#define CODE_GEN_AVG_BLOCK_SIZE 150
#endif
-#if defined(_ARCH_PPC) \
+#if defined(_ARCH_PPC) || defined(__sparc__)\
|| defined(__x86_64__) || defined(__i386__) \
-|| defined(__sparc__) || defined(__aarch64__) \
|| defined(__s390x__) || defined(__mips__) \
|| defined(CONFIG_TCG_INTERPRETER)
/* NOTE: Direct jump patching must be atomic to be thread-safe. */
@@ -398,9 +397,6 @@ static inline void tb_set_jmp_target1(uintptr_t jmp_addr,
uintptr_t addr)
atomic_set((int32_t *)jmp_addr, disp / 2);
/* no need to flush icache explicitly */
}
-#elif defined(__aarch64__)
-void aarch64_tb_set_jmp_target(uintptr_t jmp_addr, uintptr_t addr);
-#define tb_set_jmp_target1 aarch64_tb_set_jmp_target
#elif defined(__sparc__) || defined(__mips__)
void tb_set_jmp_target1(uintptr_t jmp_addr, uintptr_t addr);
#else
diff --git a/tcg/aarch64/tcg-target.inc.c b/tcg/aarch64/tcg-target.inc.c
index e488aacadb..cc81a0d5ff 100644
--- a/tcg/aarch64/tcg-target.inc.c
+++ b/tcg/aarch64/tcg-target.inc.c
@@ -865,15 +865,6 @@ static inline void tcg_out_call(TCGContext *s,
tcg_insn_unit *target)
}
}
-void aarch64_tb_set_jmp_target(uintptr_t jmp_addr, uintptr_t addr)
-{
-tcg_insn_unit *code_ptr = (tcg_insn_unit *)jmp_addr;
-tcg_insn_unit *target = (tcg_insn_unit *)addr;
-
-reloc_pc26_atomic(code_ptr, target);
-flush_icache_range(jmp_addr, jmp_addr + 4);
-}
-
static inline void tcg_out_goto_label(TCGContext *s, TCGLabel *l)
{
if (!l->has_value) {
@@ -1385,16 +1376,12 @@ static void tcg_out_op(TCGContext *s, TCGOpcode opc,
break;
case INDEX_op_goto_tb:
-#ifndef USE_DIRECT_JUMP
-#error "USE_DIRECT_JUMP required for aarch64"
-#endif
-/* consistency for USE_DIRECT_JUMP */
-tcg_debug_assert(s->tb_jmp_insn_offset != NULL);
-s->tb_jmp_insn_offset[a0] = tcg_current_code_size(s);
-/* actual branch destination will be patched by
- aarch64_tb_set_jmp_target later, beware retranslation. */
-tcg_out_goto_noaddr(s);
-s->tb_jmp_reset_offset[a0] = tcg_current_code_size(s);
+{
+intptr_t offset = tcg_pcrel_diff(s, (s->tb_jmp_target_addr + a0))
>> 2;
+tcg_out_insn(s, 3305, LDR, offset, TCG_REG_TMP);
+tcg_out_callr(s, TCG_REG_TMP);
+s->tb_jmp_reset_offset[a0] = tcg_current_code_size(s);
+}
break;
case INDEX_op_goto_ptr:
diff --git a/translate-all.c b/translate-all.c
index 966747ad60..e4cd849931 100644
--- a/translate-all.c
+++ b/translate-all.c
@@ -521,8 +521,6 @@ static inline PageDesc *page_find(tb_page_addr_t index)
# define MAX_CODE_GEN_BUFFER_SIZE (2ul * 1024 * 1024 * 1024)
#elif defined(__powerpc__)
# define MAX_CODE_GEN_BUFFER_SIZE (32u * 1024 * 1024)
-#elif defined(__aarch64__)
-# define MAX_CODE_GEN_BUFFER_SIZE (128ul * 1024 * 1024)
#elif defined(__s390x__)
/* We have a +- 4GB range on the branches; leave some slop. */
# define MAX_CODE_GEN_BUFFER_SIZE (3ul * 1024 * 1024 * 1024)
--
2.13.0
Re: [Qemu-devel] GSoC 2017 Proposal: TCG performance enhancements
On Wed, Jun 07, 2017 at 18:45:10 +0300, Lluís Vilanova wrote: > If there is such renewed interest, I will carve a bit more time to bring the > patches up to date and send the instrumentation ones for further discussion. I'm very interested and have time to spend on it -- I'm working on a simulator backend and would like to move ASAP from QSim[1] to qemu proper for the front-end. BTW I left some comments/questions a few days ago on the v7 patchset you sent in January (ouch!). I can also help with testing or bringing patches up to date -- let me know if you need any help. Thanks, Emilio [1] http://manifold.gatech.edu/projects/qsim-a-multicore-emulator/
[Qemu-devel] [PATCH] block: change variable names in BlockDriverState
Change the 'int count' parameter in bdrv_co_pwrite_zeros and
bdrv_co_pdiscard to 'int bytes', as they both refer to bytes.
This helps with code legibility.
Signed-off-by: Manos Pitsidianakis
---
include/block/block_int.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/block/block_int.h b/include/block/block_int.h
index cb78c4f..06e398b 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -162,9 +162,9 @@ struct BlockDriver {
* will be called instead.
*/
int coroutine_fn (*bdrv_co_pwrite_zeroes)(BlockDriverState *bs,
-int64_t offset, int count, BdrvRequestFlags flags);
+int64_t offset, int bytes, BdrvRequestFlags flags);
int coroutine_fn (*bdrv_co_pdiscard)(BlockDriverState *bs,
-int64_t offset, int count);
+int64_t offset, int bytes);
/*
* Building block for bdrv_block_status[_above]. The driver should
--
2.11.0
Re: [Qemu-devel] [PATCH RFC] block: add throttle block filter driver
On 06/07/2017 05:08 PM, Manos Pitsidianakis wrote: > On Wed, Jun 07, 2017 at 04:38:43PM -0500, Eric Blake wrote: >> On 06/07/2017 11:06 AM, Manos Pitsidianakis wrote: >>> On Wed, Jun 07, 2017 at 10:40:08AM -0500, Eric Blake wrote: On 06/07/2017 08:06 AM, Manos Pitsidianakis wrote: > This is part of my work for my GSOC project this summer. > > I am not sure if the count parameter in bdrv_co_pwrite_zeros and > bdrv_co_pdiscard refers to sectors or bytes. Both refer to byte counts. (We're trying to get rid of as many sector-based interfaces as possible, but it's slow going to make sure the conversions are still accurate). >>> >>> I see. Would changing the names in the BlockDriverState function >>> signatures to reflect that be okay? >> >> What names need changing? > > I meant, change "int count" to "int bytes" in the bdrv_co_pwrite_zeros > and bdrv_co_pdiscard signatures inside struct BlockDriverState. Sure, a patch to change parameter names to something that aids legibility would be welcome. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v4 1/7] util: add cacheinfo
On Wed, Jun 07, 2017 at 08:55:30 -0700, Richard Henderson wrote:
(snip)
> +#elif defined(__APPLE__) \
> + || defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
> +# include
> +# if defined(__APPLE__)
> +# define SYSCTL_CACHELINE_NAME "hw.cachelinesize"
> +# else
> +# define SYSCTL_CACHELINE_NAME "machdep.cacheline_size"
> +# endif
> +
> +static void sys_cache_info(void)
> +{
> +/* There's only a single sysctl for both I/D cache line sizes. */
> +size_t len = sizeof(qemu_icache_linesize);
> +if (!sysctlbyname(SYSCTL_CACHELINE_NAME, &qemu_icache_linesize,
> + &len, NULL, 0)) {
> +qemu_dcache_linesize = qemu_icache_linesize;
> +}
> +}
This doesn't work on the MacOS (Darwin 16.6.0) I have access to.
If we do
long size; // <-- type here matters!
size_t len = sizeof(size);
sysctlbyname(..., &size, &len, ...);
then size == 64.
However, if instead of 'long size' we have 'int size' (as in the patch),
then 'size == 1' yet sysctlbyname still returns 0. (!)
This is why I was using an intermediate long in my previous patch,
although obviously a comment there would have been appropriate.
FWIW, if we query what length sysctlbyname expects for this param (by
calling sysctlbyname(NAME, NULL, &size, NULL, 0), we get len == 8;
I presume on both 32 and 64-bit systems passing a long will work
OK here.
E.
Re: [Qemu-devel] [PATCH RFC] block: add throttle block filter driver
On Wed, Jun 07, 2017 at 04:38:43PM -0500, Eric Blake wrote: On 06/07/2017 11:06 AM, Manos Pitsidianakis wrote: On Wed, Jun 07, 2017 at 10:40:08AM -0500, Eric Blake wrote: On 06/07/2017 08:06 AM, Manos Pitsidianakis wrote: This is part of my work for my GSOC project this summer. I am not sure if the count parameter in bdrv_co_pwrite_zeros and bdrv_co_pdiscard refers to sectors or bytes. Both refer to byte counts. (We're trying to get rid of as many sector-based interfaces as possible, but it's slow going to make sure the conversions are still accurate). I see. Would changing the names in the BlockDriverState function signatures to reflect that be okay? What names need changing? I meant, change "int count" to "int bytes" in the bdrv_co_pwrite_zeros and bdrv_co_pdiscard signatures inside struct BlockDriverState. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org signature.asc Description: PGP signature
Re: [Qemu-devel] [PATCH 5/5] coccinelle: prefer glib g_new/g_renew macros
On 06/07/2017 02:46 AM, Marc-André Lureau wrote: > The g_new() familly of macros is simpler and safer than g_malloc(). s/familly/family/ > > "The return pointer is cast to the given type... Care is taken to > avoid overflow when calculating the size of the allocated block." > > I left out the common g_malloc(sizeof(*ptr)) pattern, since > alternative "g_new(typeof(*ptr))" isn't very common. But we may want > to change that too? Markus has made changes like this in the past (see commits bdd81add, b45c03f, ...). It may even be worth cribbing his commit messages, and/or converting his Coccinelle script into something stored in the repository, since we tend to re-run it and find more poor uses that have crept in over time. > > Here is the cocci script I used, then I edited manually a few > changes (I removed useless cast for ex): > > @@ > expression e1; > expression e2; > expression mem; > type t1; > @@ Your script differs from Markus', we should figure out if they can be merged into one. > ( > - g_malloc0(sizeof(*e2)) > + g_malloc0(sizeof(*e2)) Huh? > | > - g_malloc(sizeof(*e2)) > + g_malloc(sizeof(*e2)) Huh? > | > - g_realloc(mem, (e1) * sizeof(*e2)) > + g_renew(typeof(*e2), mem, e1) We haven't used typeof() very frequently. I don't know if it is worth using more frequently, maybe Markus has an opinion. > | > - g_malloc0((e1) * sizeof(*e2)) > + g_new0(typeof(*e2), e1) > | > - g_malloc((e1) * sizeof(*e2)) > + g_new(typeof(*e2), e1) > | > - g_realloc(mem, (e1) * sizeof(e2[0])) > + g_renew(typeof(e2[0]), mem, e1) Ditto. > | > - g_realloc(mem, (e1) * sizeof(e2)) > + g_renew(e2, mem, e1) This one makes sense. > Signed-off-by: Marc-André Lureau > --- > hw/lm32/lm32_hwsetup.h | 2 +- > include/hw/elf_ops.h| 2 +- > include/qemu/timer.h| 2 +- > audio/alsaaudio.c | 2 +- > audio/coreaudio.c | 2 +- > audio/dsoundaudio.c | 2 +- > audio/ossaudio.c| 2 +- > audio/paaudio.c | 2 +- > audio/wavaudio.c| 2 +- > backends/cryptodev.c| 2 +- > bootdevice.c| 2 +- > bsd-user/syscall.c | 2 +- > bt-host.c | 2 +- > bt-vhci.c | 2 +- > cpus-common.c | 4 ++-- > cpus.c | 16 > dma-helpers.c | 4 ++-- > dump.c | 10 +- > gdbstub.c | 4 ++-- > hw/9pfs/9p-handle.c | 2 +- > hw/9pfs/9p-proxy.c | 2 +- > hw/9pfs/9p-synth.c | 2 +- > hw/9pfs/9p.c| 6 +++--- > hw/9pfs/xen-9p-backend.c| 6 +++--- > hw/acpi/memory_hotplug.c| 2 +- > hw/audio/intel-hda.c| 2 +- > hw/bt/core.c| 4 ++-- > hw/bt/hci.c | 4 ++-- > hw/bt/l2cap.c | 4 ++-- > hw/bt/sdp.c | 6 +++--- > hw/char/parallel.c | 2 +- > hw/char/serial.c| 4 ++-- > hw/char/sh_serial.c | 2 +- > hw/char/virtio-serial-bus.c | 12 +--- > hw/core/irq.c | 2 +- > hw/core/ptimer.c| 2 +- > hw/core/reset.c | 2 +- > hw/cris/axis_dev88.c| 2 +- > hw/display/pxa2xx_lcd.c | 2 +- > hw/display/tc6393xb.c | 2 +- > hw/display/virtio-gpu.c | 4 ++-- > hw/display/xenfb.c | 4 ++-- > hw/dma/etraxfs_dma.c| 2 +- > hw/dma/rc4030.c | 4 ++-- > hw/dma/soc_dma.c| 6 ++ > hw/i2c/bitbang_i2c.c| 2 +- > hw/i2c/core.c | 4 ++-- > hw/i386/amd_iommu.c | 4 ++-- > hw/i386/intel_iommu.c | 2 +- > hw/i386/kvm/pci-assign.c| 2 +- > hw/i386/pc.c| 5 ++--- > hw/i386/xen/xen-hvm.c | 12 ++-- > hw/i386/xen/xen-mapcache.c | 14 +++--- > hw/input/pckbd.c| 2 +- > hw/input/ps2.c | 4 ++-- > hw/input/pxa2xx_keypad.c| 2 +- > hw/input/tsc2005.c | 3 +-- > hw/input/virtio-input.c | 4 ++-- > hw/intc/exynos4210_gic.c| 2 +- > hw/intc/heathrow_pic.c | 2 +- > hw/intc/xics.c | 2 +- > hw/intc/xics_kvm.c | 2 +- > hw/lm32/lm32_boards.c | 4 ++-- > hw/lm32/milkymist.c | 2 +- > hw/m68k/mcf5206.c | 4 ++-- > hw/m68k/mcf5208.c | 2 +- > hw/mips/mips_malta.c| 2 +- > hw/mips/mips_mipssim.c | 2 +- > h
Re: [Qemu-devel] [PATCH 2/5] coccinelle: use DIV_ROUND_UP
On 06/07/2017 02:46 AM, Marc-André Lureau wrote: > The coccinelle/round.cocci script doesn't catch hard coded values. > > I used the following script over qemu code base: > > ( > - ((e1) + 3) / (4) > + DIV_ROUND_UP(e1,4) As in 1/5, can't you also write a search for ((e1) + (e2) - 1) / e2, to cover non-constant divisions? -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH 1/5] coccinelle: replace code with ROUND_UP macro
On 06/07/2017 02:46 AM, Marc-André Lureau wrote: > I used a the following coccinelle script: > > @@ > expression e1; > @@ > - ((e1) + (3)) / (4) * (4) > + ROUND_UP(e1,4) Can't you also search for: @@ expression e1, e2 @@ - ((e1) + (e2 - 1)) / (e2) * (e2) + ROUND_UP(e1, e2) to catch cases where we are rounding by a non-constant? -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v2 3/7] qcow2: Make perform_cow() call do_perform_cow() twice
On 06/07/2017 09:08 AM, Alberto Garcia wrote:
> Instead of calling perform_cow() twice with a different COW region
> each time, call it just once and make perform_cow() handle both
> regions.
>
> This patch simply moves code around. The next one will do the actual
> reordering of the COW operations.
>
> Signed-off-by: Alberto Garcia
> ---
> block/qcow2-cluster.c | 38 +++---
> 1 file changed, 23 insertions(+), 15 deletions(-)
> qemu_co_mutex_unlock(&s->lock);
> -ret = do_perform_cow(bs, m->offset, m->alloc_offset, r->offset,
> r->nb_bytes);
> +ret = do_perform_cow(bs, m->offset, m->alloc_offset,
> + start->offset, start->nb_bytes);
> +if (ret < 0) {
> +goto fail;
> +}
> +
> +ret = do_perform_cow(bs, m->offset, m->alloc_offset,
> + end->offset, end->nb_bytes);
> +
> +fail:
Since you reach this label even on success, it feels a bit awkward. I
probably would have avoided the goto and used fewer lines by refactoring
the logic as:
ret = do_perform_cow(..., start->...);
if (ret >= 0) {
ret = do_perform_cow(..., end->...);
}
But that doesn't affect correctness, so whether or not you redo the
logic in the shorter fashion:
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH RFC] block: add throttle block filter driver
On 06/07/2017 11:06 AM, Manos Pitsidianakis wrote: > On Wed, Jun 07, 2017 at 10:40:08AM -0500, Eric Blake wrote: >> On 06/07/2017 08:06 AM, Manos Pitsidianakis wrote: >>> This is part of my work for my GSOC project this summer. >>> >>> I am not sure if the count parameter in bdrv_co_pwrite_zeros and >>> bdrv_co_pdiscard refers to sectors or bytes. >> >> Both refer to byte counts. (We're trying to get rid of as many >> sector-based interfaces as possible, but it's slow going to make sure >> the conversions are still accurate). > > I see. Would changing the names in the BlockDriverState function > signatures to reflect that be okay? What names need changing? -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH 0/3] char: fix chardev aliases regression
On 06/07/2017 01:49 PM, Marc-André Lureau wrote: > Hi, > > The patch "char: move CharBackend handling in char-fe unit" broke > chardev aliases. Here is a small series to fix it, and add a simple > unit test to check the alias keep working. > > Marc-André Lureau (3): > char: fix alias devices regression > chardev: don't use alias names in parse_compat() > test-char: start a /char/serial test > > chardev/char.c| 6 -- > tests/test-char.c | 30 ++ > 2 files changed, 34 insertions(+), 2 deletions(-) Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
[Qemu-devel] [PATCH] target/sh4: optimize cross-page and indirect jumps
Instead of unconditionally exiting to the exec loop for indirect jumps
or cross-page direct jumps, use the lookup_and_goto_ptr helper to jump
to the target if it is valid.
Signed-off-by: Aurelien Jarno
---
target/sh4/translate.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/target/sh4/translate.c b/target/sh4/translate.c
index 8bc132b27b..4f20537ef8 100644
--- a/target/sh4/translate.c
+++ b/target/sh4/translate.c
@@ -249,7 +249,7 @@ static void gen_goto_tb(DisasContext *ctx, int n,
target_ulong dest)
tcg_gen_movi_i32(cpu_pc, dest);
if (ctx->singlestep_enabled)
gen_helper_debug(cpu_env);
-tcg_gen_exit_tb(0);
+tcg_gen_lookup_and_goto_ptr(cpu_pc);
}
}
@@ -262,7 +262,7 @@ static void gen_jump(DisasContext * ctx)
tcg_gen_discard_i32(cpu_delayed_pc);
if (ctx->singlestep_enabled)
gen_helper_debug(cpu_env);
- tcg_gen_exit_tb(0);
+tcg_gen_lookup_and_goto_ptr(cpu_pc);
} else {
gen_goto_tb(ctx, 0, ctx->delayed_pc);
}
--
2.11.0
Re: [Qemu-devel] [PATCH 2/4] egl-headless: use framebuffer helper functions.
Hi
The patch looks good to me, but I tried to use egl-headless for the first
time, and I get a weird crash on virgl init:
(gdb) bt
#0 0x7fffd8cd935f in rawmemchr () at /lib64/libc.so.6
#1 0x7fffd8cc1832 in _IO_str_init_static_internal () at
/lib64/libc.so.6
#2 0x7fffd8cb37e7 in vsscanf () at /lib64/libc.so.6
#3 0x76e818a6 in vsscanf () at /lib64/libasan.so.3
#4 0x76e819d3 in sscanf () at /lib64/libasan.so.3
#5 0x76bfe239 in vrender_get_glsl_version
(glsl_version=0x60e1f2b8) at vrend_renderer.c:6026
#6 0x76bfe239 in vrend_create_context (id=id@entry=0,
nlen=nlen@entry=0, debug_name=debug_name@entry=0x0) at vrend_renderer.c:4033
#7 0x76c04f2f in vrend_renderer_context_create_internal (handle=0,
nlen=0, debug_name=0x0) at vrend_decode.c:1066
#8 0x76bf72a3 in vrend_renderer_init (cbs=,
flags=0) at vrend_renderer.c:3874
#9 0x56b52cc1 in virtio_gpu_virgl_init (g=0x63308d10) at
/home/elmarco/src/qemu/hw/display/virtio-gpu-3d.c:623
This doesn't happen when I use virgl/spice (gl=on obviously)
I guess you don't know what's going on, and I will look further later, I
just thought it was worth to report in case you had an idea.
On Tue, Jun 6, 2017 at 3:05 PM Gerd Hoffmann wrote:
> Signed-off-by: Gerd Hoffmann
---
> ui/egl-headless.c | 67
> ++-
> 1 file changed, 17 insertions(+), 50 deletions(-)
>
> diff --git a/ui/egl-headless.c b/ui/egl-headless.c
> index d8d800f8a6..809bfde99c 100644
> --- a/ui/egl-headless.c
> +++ b/ui/egl-headless.c
> @@ -8,14 +8,13 @@
> typedef struct egl_dpy {
> DisplayChangeListener dcl;
> DisplaySurface *ds;
> -int width, height;
> -GLuint texture;
> -GLuint framebuffer;
> -GLuint blit_texture;
> -GLuint blit_framebuffer;
> +egl_fb guest_fb;
> +egl_fb blit_fb;
> bool y_0_top;
> } egl_dpy;
>
> +/* -- */
> +
> static void egl_refresh(DisplayChangeListener *dcl)
> {
> graphic_hw_update(dcl->con);
> @@ -38,8 +37,8 @@ static void egl_scanout_disable(DisplayChangeListener
> *dcl)
> {
> egl_dpy *edpy = container_of(dcl, egl_dpy, dcl);
>
> -edpy->texture = 0;
> -/* XXX: delete framebuffers here ??? */
> +egl_fb_destroy(&edpy->guest_fb);
> +egl_fb_destroy(&edpy->blit_fb);
> }
>
> static void egl_scanout_texture(DisplayChangeListener *dcl,
> @@ -52,34 +51,17 @@ static void egl_scanout_texture(DisplayChangeListener
> *dcl,
> {
> egl_dpy *edpy = container_of(dcl, egl_dpy, dcl);
>
> -edpy->texture = backing_id;
> edpy->y_0_top = backing_y_0_top;
>
> /* source framebuffer */
> -if (!edpy->framebuffer) {
> -glGenFramebuffers(1, &edpy->framebuffer);
> -}
> -glBindFramebuffer(GL_FRAMEBUFFER_EXT, edpy->framebuffer);
> -glFramebufferTexture2DEXT(GL_FRAMEBUFFER_EXT,
> GL_COLOR_ATTACHMENT0_EXT,
> - GL_TEXTURE_2D, edpy->texture, 0);
> +egl_fb_create_for_tex(&edpy->guest_fb,
> + backing_width, backing_height, backing_id);
>
> /* dest framebuffer */
> -if (!edpy->blit_framebuffer) {
> -glGenFramebuffers(1, &edpy->blit_framebuffer);
> -glGenTextures(1, &edpy->blit_texture);
> -edpy->width = 0;
> -edpy->height = 0;
> -}
> -if (edpy->width != backing_width || edpy->height != backing_height) {
> -edpy->width = backing_width;
> -edpy->height = backing_height;
> -glBindTexture(GL_TEXTURE_2D, edpy->blit_texture);
> -glTexImage2D(GL_TEXTURE_2D, 0, GL_RGB,
> - edpy->width, edpy->height,
> - 0, GL_BGRA, GL_UNSIGNED_BYTE, 0);
> -glBindFramebuffer(GL_FRAMEBUFFER_EXT, edpy->blit_framebuffer);
> -glFramebufferTexture2DEXT(GL_FRAMEBUFFER_EXT,
> GL_COLOR_ATTACHMENT0_EXT,
> - GL_TEXTURE_2D, edpy->blit_texture, 0);
> +if (edpy->blit_fb.width != backing_width ||
> +edpy->blit_fb.height != backing_height) {
> +egl_fb_destroy(&edpy->blit_fb);
> +egl_fb_create_new_tex(&edpy->blit_fb, backing_width,
> backing_height);
> }
> }
>
> @@ -88,32 +70,17 @@ static void egl_scanout_flush(DisplayChangeListener
> *dcl,
>uint32_t w, uint32_t h)
> {
> egl_dpy *edpy = container_of(dcl, egl_dpy, dcl);
> -GLuint y1, y2;
>
> -if (!edpy->texture || !edpy->ds) {
> +if (!edpy->guest_fb.texture || !edpy->ds) {
> return;
> }
> -assert(surface_width(edpy->ds) == edpy->width);
> -assert(surface_height(edpy->ds) == edpy->height);
> +assert(surface_width(edpy->ds) == edpy->guest_fb.width);
> +assert(surface_height(edpy->ds) == edpy->guest_fb.height);
> assert(surface_format(edpy->ds) == PIXMAN_x8r8g8b8);
>
> -/* blit framebuffer, flip if needed */
> -glBindFramebuffer(GL_READ_FRAMEBUFFE
Re: [Qemu-devel] [PATCH] Makefile: Do not generate files if "configure" has not been run yet
On 06/07/2017 02:11 PM, Thomas Huth wrote: > When doing a "make -j10" in the vanilla QEMU source tree (without > running "configure first), the Makefile currently generates two s/"configure/"configure"/ > files already, qemu-version.h and qemu-options.def. This should not > happen, so let's make these targets depend on config-host.mak. > Also the python files can not be executed without $(PYTHON), so > these scripts should depend on config-host.mak, too. > > Signed-off-by: Thomas Huth > --- > Makefile | 5 - > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/Makefile b/Makefile > index c830d7a..6786dc2 100644 > --- a/Makefile > +++ b/Makefile > @@ -286,7 +286,7 @@ endif > > all: $(DOCS) $(TOOLS) $(HELPERS-y) recurse-all modules > > -qemu-version.h: FORCE > +qemu-version.h: config-host.mak FORCE This one makes sense. > @@ -393,6 +394,8 @@ gen-out-type = $(subst .,-,$(suffix $@)) > > qapi-py = $(SRC_PATH)/scripts/qapi.py $(SRC_PATH)/scripts/ordereddict.py > > +$(qapi-py): config-host.mak But this one is weird. How can a pre-existing file have a dependency? Remember, $(qapi-py) is not the list of built files, but the list of files used to build other files. It seems like you either want config-host.mak includes in $(qapi-py), or... > + > qga/qapi-generated/qga-qapi-types.c qga/qapi-generated/qga-qapi-types.h :\ > $(SRC_PATH)/qga/qapi-schema.json $(SRC_PATH)/scripts/qapi-types.py $(qapi-py) ...that THIS should be the rule that depends on config-host.mak in addition do depending on $(qapi-py). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v3 4/5] xics: directly register ICPState objects to vmstate
On Wed, 7 Jun 2017 20:14:01 +0200
Cédric Le Goater wrote:
> On 06/07/2017 07:17 PM, Greg Kurz wrote:
> > The ICPState objects are currently registered to vmstate as qdev objects.
> > Their instance ids are hence computed automatically in the migration code,
> > and thus depends on the order the CPU cores were plugged.
> >
> > If the destination had its CPU cores plugged in a different order than the
> > source, then ICPState objects will have different instance_ids and load
> > the wrong state.
> >
> > Since CPU objects have a reliable cpu_index which is already used as
> > instance_id in vmstate, let's use it for ICPState as well.
> >
> > Signed-off-by: Greg Kurz
> > ---
> > hw/intc/xics.c |5 -
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> > index 330441ff7fe8..3d76b43876c5 100644
> > --- a/hw/intc/xics.c
> > +++ b/hw/intc/xics.c
> > @@ -348,10 +348,14 @@ static void icp_realize(DeviceState *dev, Error
> > **errp)
> > }
> >
> > qemu_register_reset(icp_reset, dev);
> > +vmstate_register(NULL, icp->cs->cpu_index, &vmstate_icp_server, icp);
>
> so, what I just said in the previous email regarding ->cs would break this
> patch. Forget about it.
>
Unless we have an icp->cpu_index like I mentioned in the other mail.
>
> Reviewed-by: Cédric Le Goater
>
> Thanks,
>
> C.
>
> > }
> >
> > static void icp_unrealize(DeviceState *dev, Error **errp)
> > {
> > +ICPState *icp = ICP(dev);
> > +
> > +vmstate_unregister(NULL, &vmstate_icp_server, icp);
> > qemu_unregister_reset(icp_reset, dev);
> > }
> >
> > @@ -359,7 +363,6 @@ static void icp_class_init(ObjectClass *klass, void
> > *data)
> > {
> > DeviceClass *dc = DEVICE_CLASS(klass);
> >
> > -dc->vmsd = &vmstate_icp_server;
> > dc->realize = icp_realize;
> > dc->unrealize = icp_unrealize;
> > }
> >
>
pgps9OTKIGHzN.pgp
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v3 3/5] xics: setup cpu at realize time
On Wed, 7 Jun 2017 20:11:38 +0200
Cédric Le Goater wrote:
> On 06/07/2017 07:17 PM, Greg Kurz wrote:
> > Until recently, spapr used to allocate ICPState objects for the lifetime
> > of the machine. They would only be associated to vCPUs in xics_cpu_setup()
> > when plugging a CPU core.
> >
> > Now that ICPState objects have the same lifecycle as vCPUs, it is
> > possible to associate them during realization.
> >
> > This patch hence open-codes xics_cpu_setup() in icp_realize(). The vCPU
> > is passed as a property. Note that vCPU now needs to be realized first
> > for the IRQs to be allocated. It also needs to resetted before ICPState
> > realization in order to synchronize with KVM.
> >
> > Since ICPState objects are freed when unrealized, xics_cpu_destroy() isn't
> > needed anymore and can be safely dropped.
>
> I like the idea but I think the assignment of ->cs attribute should be
> moved under icp_kvm_cpu_setup(), as it is only used under xics_kvm by
> the kvm_vcpu_ioctl() calls.
>
Well, cs->cpu_index is also used for traces and to implement the 'info pic'
monitor command.
> Ideally, we should also introduce :
>
> struct KvmState {
> ICPState parent_obj;
>
> CPUState *cs;
> };
>
> That would be cleaner, but that might introduce some migration compat
> issues again ...
>
No, as long as it doesn't change state, we're good. My concern is more
about how to pass cs to xics_kvm and the cs->cpu_index to xics.
> Some minor comments below,
>
> Thanks,
>
> C.
>
> > Signed-off-by: Greg Kurz
> > ---
> > hw/intc/xics.c | 76
> > ---
> > hw/ppc/pnv_core.c | 16 --
> > hw/ppc/spapr_cpu_core.c | 21 ++---
> > include/hw/ppc/xics.h |2 -
> > 4 files changed, 48 insertions(+), 67 deletions(-)
> >
> > diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> > index ec73f02144c9..330441ff7fe8 100644
> > --- a/hw/intc/xics.c
> > +++ b/hw/intc/xics.c
> > @@ -38,50 +38,6 @@
> > #include "monitor/monitor.h"
> > #include "hw/intc/intc.h"
> >
> > -void xics_cpu_destroy(XICSFabric *xi, PowerPCCPU *cpu)
> > -{
> > -CPUState *cs = CPU(cpu);
> > -ICPState *icp = ICP(cpu->intc);
> > -
> > -assert(icp);
> > -assert(cs == icp->cs);
> > -
> > -icp->output = NULL;
> > -icp->cs = NULL;
> > -}
> > -
> > -void xics_cpu_setup(XICSFabric *xi, PowerPCCPU *cpu, ICPState *icp)
> > -{
> > -CPUState *cs = CPU(cpu);
> > -CPUPPCState *env = &cpu->env;
> > -ICPStateClass *icpc;
> > -
> > -assert(icp);
> > -
> > -cpu->intc = OBJECT(icp);
> > -icp->cs = cs;
> > -
> > -icpc = ICP_GET_CLASS(icp);
> > -if (icpc->cpu_setup) {
> > -icpc->cpu_setup(icp, cpu);
> > -}
> > -
> > -switch (PPC_INPUT(env)) {
> > -case PPC_FLAGS_INPUT_POWER7:
> > -icp->output = env->irq_inputs[POWER7_INPUT_INT];
> > -break;
> > -
> > -case PPC_FLAGS_INPUT_970:
> > -icp->output = env->irq_inputs[PPC970_INPUT_INT];
> > -break;
> > -
> > -default:
> > -error_report("XICS interrupt controller does not support this CPU "
> > - "bus model");
> > -abort();
> > -}
> > -}
> > -
> > void icp_pic_print_info(ICPState *icp, Monitor *mon)
> > {
> > int cpu_index = icp->cs ? icp->cs->cpu_index : -1;
> > @@ -343,6 +299,8 @@ static void icp_realize(DeviceState *dev, Error **errp)
> > {
> > ICPState *icp = ICP(dev);
> > ICPStateClass *icpc = ICP_GET_CLASS(dev);
> > +PowerPCCPU *cpu;
> > +CPUPPCState *env;
> > Object *obj;
> > Error *err = NULL;
> >
> > @@ -355,6 +313,36 @@ static void icp_realize(DeviceState *dev, Error **errp)
> >
> > icp->xics = XICS_FABRIC(obj);
> >
> > +obj = object_property_get_link(OBJECT(dev), "cs", &err);
> > +if (!obj) {
> > +error_setg(errp, "%s: required link 'xics' not found: %s",
> > + __func__, error_get_pretty(err));
> > +return;
> > +}
> > +
> > +cpu = POWERPC_CPU(obj);
> > +cpu->intc = OBJECT(icp);
> > +icp->cs = CPU(obj);
>
> only xics_kvm needs ->cs.
>
yeah, maybe the base class should only have a cpu_index field:
icp->cpu_index = CPU(obj)->cpu_index;
> > +
> > +if (icpc->cpu_setup) {
> > +icpc->cpu_setup(icp, cpu);
> > +}
> > +
> > +env = &cpu->env;
> > +switch (PPC_INPUT(env)) {
> > +case PPC_FLAGS_INPUT_POWER7:
> > +icp->output = env->irq_inputs[POWER7_INPUT_INT];
> > +break;
> > +
> > +case PPC_FLAGS_INPUT_970:
> > +icp->output = env->irq_inputs[PPC970_INPUT_INT];
> > +break;
> > +
> > +default:
> > +error_setg(errp, "XICS interrupt controller does not support this
> > CPU bus model");
> > +return;
> > +}
> > +
> > if (icpc->realize) {
> > icpc->realize(dev, errp);
> > }
> > diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
> > index e8a9
Re: [Qemu-devel] [PATCH v3 00/43] qobject/qapi: add uint type
Hi,
This series failed automatic build test. Please find the testing commands and
their output below. If you have docker installed, you can probably reproduce it
locally.
Type: series
Subject: [Qemu-devel] [PATCH v3 00/43] qobject/qapi: add uint type
Message-id: 20170607163635.17635-1-marcandre.lur...@redhat.com
=== TEST SCRIPT BEGIN ===
#!/bin/bash
set -e
git submodule update --init dtc
# Let docker tests dump environment info
export SHOW_ENV=1
export J=8
time make docker-test-quick@centos6
time make docker-test-mingw@fedora
time make docker-test-build@min-glib
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
5304b17 qobject: move dump_qobject() from block/ to qobject/
04e93f9 RFC: qdict: add uint
b1f8183 tests/qdict: check more get_try_int() cases
32dc113 console: use get_uint() for "head" property
ccdbdd6 i386/cpu: use get_uint() for "min-level"/"min-xlevel" properties
eabad51 numa: use get_uint() for "size" property
23ad49e pnv-core: use get_uint() for "core-pir" property
c327b58 pvpanic: use get_uint() for "ioport" property
2af440d auxbus: use get_uint() for "addr" property
63db11b arm: use get_uint() for "mp-affinity" property
ede6718 xen: use get_uint() for "max-ram-below-4g" property
d00a748 pc: use get_uint() for "hpet-intcap" property
2be46be pc: use get_uint() for "apic-id" property
0bbec76 pc: use get_uint() for "iobase" property
54613bf acpi: use get_uint() for "pci-hole*" properties
a9029ef acpi: use get_uint() for various acpi properties
4c2ac9c acpi: use get_uint() for "acpi-pcihp-io*" properties
5890942 platform-bus: use get_uint() for "addr" property
16b2431 bcm2835_fb: use {get, set}_uint() for "vcram-size" and "vcram-base"
52a5c50 aspeed: use {set, get}_uint() for "ram-size" property
8ee5ed0 pcihp: use get_uint() for "bsel" property
ec9a2db pc-dimm: make "size" property uint64
5b65717 pc-dimm: use get_uint() for dimm properties
e05a02f isa: use get_uint() for "io-base"
74e0f4b qdev: use appropriate getter/setters type
ea4b431 apic-common: make "id" property a uint32
917ff10 qdev: avoid type casts between signed and unsigned
c803fcd qdev: wrap default property value in an union
0153836 qdev: rename DEFINE_PROP_DEFAULT() to DEFINE_PROP_SIGNED()
00b6346 object: use more specific property type names
6ebdb40 q35: fix get_mmcfg_size to use uint64 visitor
b66da0f object: add uint property setter/getter
3123cbf qapi: update the qobject visitor to use QNUM_U64
f65fbd4 json: learn to parse uint64 numbers
cbe9a26 qnum: add uint type
47d9e93 tests: remove /{qnum, qlist, dict}/destroy test
19e22e7 qapi: Remove visit_start_alternate() parameter promote_int
8564a30 qapi: merge QInt and QFloat in QNum
3a28906 qapi: minor refactoring
5c73880 tests: add more int/number ranges checks
98c8358 tests: Remove test cases for alternates of 'number' and 'int'
d2fce78 object: fix potential leak in getters
0c9c6c3 qdev: remove PropertyInfo.qtype field
=== OUTPUT BEGIN ===
Submodule 'dtc' (git://git.qemu-project.org/dtc.git) registered for path 'dtc'
Cloning into '/var/tmp/patchew-tester-tmp-gg351w48/src/dtc'...
Submodule path 'dtc': checked out '558cd81bdd432769b59bff01240c44f82cfb1a9d'
BUILD centos6
make[1]: Entering directory '/var/tmp/patchew-tester-tmp-gg351w48/src'
ARCHIVE qemu.tgz
ARCHIVE dtc.tgz
COPYRUNNER
RUN test-quick in qemu:centos6
Packages installed:
SDL-devel-1.2.14-7.el6_7.1.x86_64
ccache-3.1.6-2.el6.x86_64
epel-release-6-8.noarch
gcc-4.4.7-17.el6.x86_64
git-1.7.1-4.el6_7.1.x86_64
glib2-devel-2.28.8-5.el6.x86_64
libfdt-devel-1.4.0-1.el6.x86_64
make-3.81-23.el6.x86_64
package g++ is not installed
pixman-devel-0.32.8-1.el6.x86_64
tar-1.23-15.el6_8.x86_64
zlib-devel-1.2.3-29.el6.x86_64
Environment variables:
PACKAGES=libfdt-devel ccache tar git make gcc g++ zlib-devel
glib2-devel SDL-devel pixman-devel epel-release
HOSTNAME=07e2c7a877f4
TERM=xterm
MAKEFLAGS= -j8
HISTSIZE=1000
J=8
USER=root
CCACHE_DIR=/var/tmp/ccache
EXTRA_CONFIGURE_OPTS=
V=
SHOW_ENV=1
MAIL=/var/spool/mail/root
PATH=/usr/lib/ccache:/usr/lib64/ccache:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
PWD=/
LANG=en_US.UTF-8
TARGET_LIST=
HISTCONTROL=ignoredups
SHLVL=1
HOME=/root
TEST_DIR=/tmp/qemu-test
LOGNAME=root
LESSOPEN=||/usr/bin/lesspipe.sh %s
FEATURES= dtc
DEBUG=
G_BROKEN_FILENAMES=1
CCACHE_HASHDIR=
_=/usr/bin/env
Configure options:
--enable-werror --target-list=x86_64-softmmu,aarch64-softmmu
--prefix=/var/tmp/qemu-build/install
No C++ compiler available; disabling C++ specific optional code
Install prefix/var/tmp/qemu-build/install
BIOS directory/var/tmp/qemu-build/install/share/qemu
binary directory /var/tmp/qemu-build/install/bin
library directory /var/tmp/qemu-build/install/lib
module directory /var/tmp/qemu-build/install/lib/qemu
libexec directory /var/tmp/qemu-build/install/libexec
include directory /var/tmp/qemu-build/install/include
config directory /var/tmp/qemu-build/install/etc
local state directory /var
Re: [Qemu-devel] [PULL 22/26] target/aarch64: optimize indirect branches
On 06/07/2017 07:11 AM, Alex Bennée wrote: Richard Henderson writes: From: "Emilio G. Cota" Measurements: [Baseline performance is that before applying this and the previous commit] Sadly this has regressed my qemu-system-aarch64 EL2 run. It was slightly masked by an unrelated assertion breakage which I had to fix. However with this patch my boot hangs spinning all 4 threads. Once reverted things work again. My command line: timeout -k 1s --foreground 120s ./aarch64-softmmu/qemu-system-aarch64 -machine type=virt -display none -m 4096 -cpu cortex-a57 -serial mon:stdio -netdev user,id=unet -device virtio-net-device,netdev=unet -drive file=/home/alex/lsrc/qemu/images/jessie-arm64.qcow2,id=myblock,index=0,if=none -device virtio-blk-device,drive=myblock -append "console=ttyAMA0 root=/dev/vda1 systemd.unit=benchmark.service" -kernel /home/alex/lsrc/qemu/images/aarch64-current-linux-kernel-only.img -smp 4 -machine gic-version=3 -machine virtualization=true -name debug-threads=on If you ignore the timeout, does it complete eventually? It may not be the same thing, but that pull left out the final patch for target/alpha that enabled goto_ptr. But as I hadn't seen a similar problem with other guests, I was assuming that the problem was in hw/alpha/, or some other bit of the interrupt chain. r~
Re: [Qemu-devel] [PULL 22/26] target/aarch64: optimize indirect branches
On Wed, Jun 07, 2017 at 15:22:48 +0100, Alex Bennée wrote: > > Alex Bennée writes: > > > Richard Henderson writes: > > > >> From: "Emilio G. Cota" > >> > >> Measurements: > >> > >> [Baseline performance is that before applying this and the previous > >> commit] > > > > Sadly this has regressed my qemu-system-aarch64 EL2 run. It was slightly > > masked by an unrelated assertion breakage which I had to fix. However > > with this patch my boot hangs spinning all 4 threads. Once reverted > > things work again. > > > > My command line: > > > > timeout -k 1s --foreground 120s ./aarch64-softmmu/qemu-system-aarch64 > > -machine type=virt -display none -m 4096 -cpu cortex-a57 -serial mon:stdio > > -netdev user,id=unet -device virtio-net-device,netdev=unet -drive > > file=/home/alex/lsrc/qemu/images/jessie-arm64.qcow2,id=myblock,index=0,if=none > > -device virtio-blk-device,drive=myblock -append "console=ttyAMA0 > > root=/dev/vda1 systemd.unit=benchmark.service" -kernel > > /home/alex/lsrc/qemu/images/aarch64-current-linux-kernel-only.img -smp 4 > > -machine gic-version=3 -machine virtualization=true -name debug-threads=on > > > > My tree with fix and revert: > > > > https://github.com/stsquad/qemu/tree/debug/aarch64-hang > > > > I'm investigating now. > > Well this seems to be a case of hangs with -smp > 1 (which I guess was > obvious seeing as the TCG threads seem to be spinning against each > other). Not sure the problem is in MTTCG; I cannot reproduce with -smp > 1 and thread=single; can you? [ I get no output whatsoever when trying to boot ] Note that you'll need to revert bde4d9205ee to get thread=foo to work again -- see [1]. Thanks, E. [1] https://lists.gnu.org/archive/html/qemu-devel/2017-06/msg01980.html
Re: [Qemu-devel] [PATCH v2] Fix the -accel parameter and the documentation for 'hax'
On Thu, May 04, 2017 at 09:11:50 +0200, Markus Armbruster wrote:
> Thomas Huth writes:
(snip)
> > STEXI
> > @item -accel @var{name}[,prop=@var{value}[,...]]
> > @findex -accel
> > This is used to enable an accelerator. Depending on the target
> > architecture,
> > -kvm, xen, or tcg can be available. By default, tcg is used. If there is
> > more
> > -than one accelerator specified, the next one is used if the previous one
> > fails
> > -to initialize.
> > +kvm, xen, hax or tcg can be available. By default, tcg is used. If there is
> > +more than one accelerator specified, the next one is used if the previous
> > one
> > +fails to initialize.
> > @table @option
> > @item thread=single|multi
> > Controls number of TCG threads. When the TCG is multi-threaded there will
> > be one
> > diff --git a/vl.c b/vl.c
> > index f46e070..0a1b931 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -3725,26 +3725,21 @@ int main(int argc, char **argv, char **envp)
> > qdev_prop_register_global(&kvm_pit_lost_tick_policy);
> > break;
> > }
> > -case QEMU_OPTION_accel:
> > +case QEMU_OPTION_accel: {
> > +QemuOpts *accel_opts;
>
> Doesn't this shadow the @accel_opts declared in main()'s outermost
> scope?
Yes, it does :( Unfortunately Markus' review slipped through the
cracks and this patch ended up upstream (bde4d9205). It causes
a regression that breaks qemu_tcg_configure(accel_opts)
since now accel_opts is always NULL. That is, in `-accel [..],thread=foo'
foo is ignored.
Emilio
[Qemu-devel] [Bug 1696180] Re: Issues with qemu-img, libgfapi, and encryption at rest
** Changed in: qemu Status: Incomplete => Triaged -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1696180 Title: Issues with qemu-img, libgfapi, and encryption at rest Status in QEMU: Triaged Bug description: Hi, Encryption-at-rest has been supported for some time now. The client is responsible for encrypting the files with a help of a master key file. I have a properly setup environment and everything appears to be working fine but when I use qemu-img to move a file to gluster I get the following: # qemu-img convert -f raw -O raw linux.iso gluster://gluster01/virt0/linux.raw [2017-06-06 16:52:25.489720] E [mem-pool.c:579:mem_put] (-->/lib64/libglusterfs.so.0(syncop_lookup+0x4e5) [0x7f30f7a36d35] -->/lib64/libglusterfs.so.0(+0x59f02) [0x7f30f7a32f02] -->/lib64/libglusterfs.so.0(mem_put+0x190) [0x7f30f7a24a60] ) 0-mem-pool: mem-pool ptr is NULL [2017-06-06 16:52:25.490778] E [mem-pool.c:579:mem_put] (-->/lib64/libglusterfs.so.0(syncop_lookup+0x4e5) [0x7f30f7a36d35] -->/lib64/libglusterfs.so.0(+0x59f02) [0x7f30f7a32f02] -->/lib64/libglusterfs.so.0(mem_put+0x190) [0x7f30f7a24a60] ) 0-mem-pool: mem-pool ptr is NULL [2017-06-06 16:52:25.492263] E [mem-pool.c:579:mem_put] (-->/lib64/libglusterfs.so.0(syncop_lookup+0x4e5) [0x7f30f7a36d35] -->/lib64/libglusterfs.so.0(+0x59f02) [0x7f30f7a32f02] -->/lib64/libglusterfs.so.0(mem_put+0x190) [0x7f30f7a24a60] ) 0-mem-pool: mem-pool ptr is NULL [2017-06-06 16:52:25.497226] E [mem-pool.c:579:mem_put] (-->/lib64/libglusterfs.so.0(syncop_create+0x44d) [0x7f30f7a3cf5d] -->/lib64/libglusterfs.so.0(+0x59f02) [0x7f30f7a32f02] -->/lib64/libglusterfs.so.0(mem_put+0x190) [0x7f30f7a24a60] ) 0-mem-pool: mem-pool ptr is NULL On and on until I get this message: [2017-06-06 17:00:03.467361] E [MSGID: 108006] [afr-common.c:4409:afr_notify] 0-virt0-replicate-0: All subvolumes are down. Going offline until atleast one of them comes back up. [2017-06-06 17:00:03.467442] E [MSGID: 108006] [afr-common.c:4409:afr_notify] 0-virt0-replicate-1: All subvolumes are down. Going offline until atleast one of them comes back up. I asked for help assuming it's a problem with glusterfs and was told it appears qemu-img's implementation of libgfapi doesn't call the xlator function correctly. To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1696180/+subscriptions
Re: [Qemu-devel] [PATCH v4 0/7] tcg: allocate TB structs preceding translate
Hi, This series failed automatic build test. Please find the testing commands and their output below. If you have docker installed, you can probably reproduce it locally. Subject: [Qemu-devel] [PATCH v4 0/7] tcg: allocate TB structs preceding translate Type: series Message-id: 20170607155536.1193-1-...@twiddle.net === TEST SCRIPT BEGIN === #!/bin/bash set -e git submodule update --init dtc # Let docker tests dump environment info export SHOW_ENV=1 export J=8 time make docker-test-quick@centos6 time make docker-test-mingw@fedora time make docker-test-build@min-glib === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 Switched to a new branch 'test' a5f5544 target/arm: Use ldr (literal) for goto_tb 15a92d8 target/arm: Try pc-relative addresses for movi c270c42 target/arm: Remove limit on code buffer size 92df92d target/arm: Use indirect branch for goto_tb ca5d3fd tcg/aarch64: Use ADR in tcg_out_movi c62a640 tcg: allocate TB structs before the corresponding translated code b3e2a1f util: add cacheinfo === OUTPUT BEGIN === Submodule 'dtc' (git://git.qemu-project.org/dtc.git) registered for path 'dtc' Cloning into '/var/tmp/patchew-tester-tmp-dlt2qyme/src/dtc'... Submodule path 'dtc': checked out '558cd81bdd432769b59bff01240c44f82cfb1a9d' BUILD centos6 make[1]: Entering directory '/var/tmp/patchew-tester-tmp-dlt2qyme/src' ARCHIVE qemu.tgz ARCHIVE dtc.tgz COPYRUNNER RUN test-quick in qemu:centos6 Packages installed: SDL-devel-1.2.14-7.el6_7.1.x86_64 ccache-3.1.6-2.el6.x86_64 epel-release-6-8.noarch gcc-4.4.7-17.el6.x86_64 git-1.7.1-4.el6_7.1.x86_64 glib2-devel-2.28.8-5.el6.x86_64 libfdt-devel-1.4.0-1.el6.x86_64 make-3.81-23.el6.x86_64 package g++ is not installed pixman-devel-0.32.8-1.el6.x86_64 tar-1.23-15.el6_8.x86_64 zlib-devel-1.2.3-29.el6.x86_64 Environment variables: PACKAGES=libfdt-devel ccache tar git make gcc g++ zlib-devel glib2-devel SDL-devel pixman-devel epel-release HOSTNAME=08601069c9dd TERM=xterm MAKEFLAGS= -j8 HISTSIZE=1000 J=8 USER=root CCACHE_DIR=/var/tmp/ccache EXTRA_CONFIGURE_OPTS= V= SHOW_ENV=1 MAIL=/var/spool/mail/root PATH=/usr/lib/ccache:/usr/lib64/ccache:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin PWD=/ LANG=en_US.UTF-8 TARGET_LIST= HISTCONTROL=ignoredups SHLVL=1 HOME=/root TEST_DIR=/tmp/qemu-test LOGNAME=root LESSOPEN=||/usr/bin/lesspipe.sh %s FEATURES= dtc DEBUG= G_BROKEN_FILENAMES=1 CCACHE_HASHDIR= _=/usr/bin/env Configure options: --enable-werror --target-list=x86_64-softmmu,aarch64-softmmu --prefix=/var/tmp/qemu-build/install No C++ compiler available; disabling C++ specific optional code Install prefix/var/tmp/qemu-build/install BIOS directory/var/tmp/qemu-build/install/share/qemu binary directory /var/tmp/qemu-build/install/bin library directory /var/tmp/qemu-build/install/lib module directory /var/tmp/qemu-build/install/lib/qemu libexec directory /var/tmp/qemu-build/install/libexec include directory /var/tmp/qemu-build/install/include config directory /var/tmp/qemu-build/install/etc local state directory /var/tmp/qemu-build/install/var Manual directory /var/tmp/qemu-build/install/share/man ELF interp prefix /usr/gnemul/qemu-%M Source path /tmp/qemu-test/src C compilercc Host C compiler cc C++ compiler Objective-C compiler cc ARFLAGS rv CFLAGS-O2 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -g QEMU_CFLAGS -I/usr/include/pixman-1 -I$(SRC_PATH)/dtc/libfdt -pthread -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -fPIE -DPIE -m64 -mcx16 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -Wendif-labels -Wno-missing-include-dirs -Wempty-body -Wnested-externs -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wold-style-declaration -Wold-style-definition -Wtype-limits -fstack-protector-all LDFLAGS -Wl,--warn-common -Wl,-z,relro -Wl,-z,now -pie -m64 -g make make install install pythonpython -B smbd /usr/sbin/smbd module supportno host CPU x86_64 host big endian no target list x86_64-softmmu aarch64-softmmu tcg debug enabled no gprof enabled no sparse enabledno strip binariesyes profiler no static build no pixmansystem SDL support yes (1.2.14) GTK support no GTK GL supportno VTE support no TLS priority NORMAL GNUTLS supportno GNUTLS rndno libgcrypt no libgcrypt kdf no nettleno nettle kdfno libtasn1 no curses supportno virgl support no curl support no mingw32 support no Audio drivers oss Block whitelist (rw) Block whitelist (ro) VirtFS supportno VNC support yes VNC SASL support no VNC JPEG support no VNC PNG support no xen support no brlapi supportno
[Qemu-devel] [PATCH] target/xtensa: gdbstub: drop dead return statement
Signed-off-by: Max Filippov --- target/xtensa/gdbstub.c | 1 - 1 file changed, 1 deletion(-) diff --git a/target/xtensa/gdbstub.c b/target/xtensa/gdbstub.c index da131ae..d78a1b4 100644 --- a/target/xtensa/gdbstub.c +++ b/target/xtensa/gdbstub.c @@ -72,7 +72,6 @@ int xtensa_cpu_gdb_read_register(CPUState *cs, uint8_t *mem_buf, int n) __func__, n, reg->type); memset(mem_buf, 0, reg->size); return reg->size; -return 0; } } -- 2.1.4
Re: [Qemu-devel] [PATCH] target/xtensa: handle unknown registers in gdbstub
On Tue, Jun 6, 2017 at 5:55 PM, Richard Henderson wrote: > On 06/03/2017 02:19 AM, Max Filippov wrote: >> >> +memset(mem_buf, 0, reg->size); >> +return reg->size; >> return 0; > > Leaving a dead return. Meh ): Thanks for the review. Will send a fix. -- Thanks. -- Max
[Qemu-devel] [Bug 1696180] Re: Issues with qemu-img, libgfapi, and encryption at rest
Just upgraded to 2.9.0 and actually I see a different issue: # qemu-img convert -O raw fedora.iso gluster://dalpinfglt04/virt0/fedora6.raw [2017-06-07 16:52:43.300902] C [rpc-clnt-ping.c:160:rpc_clnt_ping_timer_expired] 0-virt0-client-2: server 172.19.38.42:49152 has not responded in the last 42 seconds, disconnecting. [2017-06-07 17:02:44.342745] E [rpc-clnt.c:365:saved_frames_unwind] (--> /lib64/libglusterfs.so.0(_gf_log_callingfn+0x17d)[0x7f78c3e4fe6d] (--> /lib64/libgfrpc.so.0(saved_frames_unwind+0x1d1)[0x7f78c3c169a1] (--> /lib64/libgfrpc.so.0(saved_frames_destroy+0xe)[0x7f78c3c16abe] (--> /lib64/libgfrpc.so.0(rpc_clnt_connection_cleanup+0x87)[0x7f78c3c18157] (--> /lib64/libgfrpc.so.0(rpc_clnt_notify+0x288)[0x7f78c3c18c28] ) 0-virt0-client-2: forced unwinding frame type(GlusterFS 3.3) op(WRITE(13)) called at 2017-06-07 16:52:00.618744 (xid=0x1c) [2017-06-07 17:02:44.342952] E [rpc-clnt.c:365:saved_frames_unwind] (--> /lib64/libglusterfs.so.0(_gf_log_callingfn+0x17d)[0x7f78c3e4fe6d] (--> /lib64/libgfrpc.so.0(saved_frames_unwind+0x1d1)[0x7f78c3c169a1] (--> /lib64/libgfrpc.so.0(saved_frames_destroy+0xe)[0x7f78c3c16abe] (--> /lib64/libgfrpc.so.0(rpc_clnt_connection_cleanup+0x87)[0x7f78c3c18157] (--> /lib64/libgfrpc.so.0(rpc_clnt_notify+0x288)[0x7f78c3c18c28] ) 0-virt0-client-2: forced unwinding frame type(GF-DUMP) op(NULL(2)) called at 2017-06-07 16:52:00.618753 (xid=0x1d) [2017-06-07 17:02:44.343415] E [MSGID: 114031] [client-rpc-fops.c:1593:client3_3_finodelk_cbk] 0-virt0-client-2: remote operation failed [Transport endpoint is not connected] [2017-06-07 17:08:49.367264] C [rpc-clnt-ping.c:160:rpc_clnt_ping_timer_expired] 0-virt0-client-3: server 172.19.38.43:49152 has not responded in the last 42 seconds, disconnecting. [2017-06-07 17:13:29.969206] E [rpc-clnt.c:365:saved_frames_unwind] (--> /lib64/libglusterfs.so.0(_gf_log_callingfn+0x17d)[0x7f78c3e4fe6d] (--> /lib64/libgfrpc.so.0(saved_frames_unwind+0x1d1)[0x7f78c3c169a1] (--> /lib64/libgfrpc.so.0(saved_frames_destroy+0xe)[0x7f78c3c16abe] (--> /lib64/libgfrpc.so.0(rpc_clnt_connection_cleanup+0x87)[0x7f78c3c18157] (--> /lib64/libgfrpc.so.0(rpc_clnt_notify+0x288)[0x7f78c3c18c28] ) 0-virt0-client-3: forced unwinding frame type(GlusterFS 3.3) op(WRITE(13)) called at 2017-06-07 17:08:06.371259 (xid=0x22) [2017-06-07 17:13:29.969250] E [MSGID: 114031] [client-rpc-fops.c:1593:client3_3_finodelk_cbk] 0-virt0-client-3: remote operation failed [Transport endpoint is not connected] [2017-06-07 17:13:29.969355] E [rpc-clnt.c:365:saved_frames_unwind] (--> /lib64/libglusterfs.so.0(_gf_log_callingfn+0x17d)[0x7f78c3e4fe6d] (--> /lib64/libgfrpc.so.0(saved_frames_unwind+0x1d1)[0x7f78c3c169a1] (--> /lib64/libgfrpc.so.0(saved_frames_destroy+0xe)[0x7f78c3c16abe] (--> /lib64/libgfrpc.so.0(rpc_clnt_connection_cleanup+0x87)[0x7f78c3c18157] (--> /lib64/libgfrpc.so.0(rpc_clnt_notify+0x288)[0x7f78c3c18c28] ) 0-virt0-client-3: forced unwinding frame type(GF-DUMP) op(NULL(2)) called at 2017-06-07 17:08:06.371268 (xid=0x23) [2017-06-07 17:13:29.972665] E [MSGID: 108008] [afr-transaction.c:2619:afr_write_txn_refresh_done] 0-virt0-replicate-1: Failing FSETXATTR on gfid 86042280-9ae1-444f-8342-be4442f82111: split-brain observed. [Input/output error] [2017-06-07 17:13:29.977821] E [MSGID: 108008] [afr-read-txn.c:90:afr_read_txn_refresh_done] 0-virt0-replicate-1: Failing FGETXATTR on gfid 86042280-9ae1-444f-8342-be4442f82111: split-brain observed. [Input/output error] [2017-06-07 17:13:29.981667] E [MSGID: 114031] [client-rpc-fops.c:1593:client3_3_finodelk_cbk] 0-virt0-client-2: remote operation failed [Invalid argument] [2017-06-07 17:13:30.157560] E [MSGID: 108006] [afr-common.c:4781:afr_notify] 0-virt0-replicate-0: All subvolumes are down. Going offline until atleast one of them comes back up. [2017-06-07 17:13:30.157904] E [MSGID: 108006] [afr-common.c:4781:afr_notify] 0-virt0-replicate-1: All subvolumes are down. Going offline until atleast one of them comes back up. qemu-img: gluster://dalpinfglt04/virt0/fedora6.raw: error while converting raw: Could not create image: Transport endpoint is not connected The file was created but nothing was written to it. Either way, I don't think encryption-at-rest is tested much with qemu integration. -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1696180 Title: Issues with qemu-img, libgfapi, and encryption at rest Status in QEMU: Incomplete Bug description: Hi, Encryption-at-rest has been supported for some time now. The client is responsible for encrypting the files with a help of a master key file. I have a properly setup environment and everything appears to be working fine but when I use qemu-img to move a file to gluster I get the following: # qemu-img convert -f raw -O raw linux.iso gluster://gluster01/virt0/linux.raw [2017-06-06 16:52:25.48972
Re: [Qemu-devel] [PATCH] monitor: Add -a (all) option to info registers
* Suraj Jitindar Singh (sjitindarsi...@gmail.com) wrote:
> The info registers command in the qemu monitor is used to dump register
> values.
>
> Currently this command uses the monitor cpu (which can be set by the
> user) as the cpu for whose registers will be dumped. Sometimes it is
> useful to see the registers for all cpus and currently this requires
> setting the monitor cpu and the re-running the command for each cpu
> in the system. I would be nice if there was an easier way to do this.
>
> Add the "-a" option to the info registers command to dump the register
> values for all cpus.
>
> Signed-off-by: Suraj Jitindar Singh
Hi Suraj,
That looks pretty good, one minor suggestion below
> ---
> hmp-commands-info.hx | 6 +++---
> monitor.c| 20 +++-
> 2 files changed, 18 insertions(+), 8 deletions(-)
>
> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> index ae16901..ba98e58 100644
> --- a/hmp-commands-info.hx
> +++ b/hmp-commands-info.hx
> @@ -100,9 +100,9 @@ ETEXI
>
> {
> .name = "registers",
> -.args_type = "",
> -.params = "",
> -.help = "show the cpu registers",
> +.args_type = "cpustate_all:-a",
> +.params = "[-a]",
> +.help = "show the cpu registers (-a: all - show register info
> for all cpus)",
> .cmd= hmp_info_registers,
> },
>
> diff --git a/monitor.c b/monitor.c
> index baa73c9..5875f88 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -1078,13 +1078,23 @@ int monitor_get_cpu_index(void)
>
> static void hmp_info_registers(Monitor *mon, const QDict *qdict)
> {
> -CPUState *cs = mon_get_cpu();
> +bool all_cpus = qdict_get_try_bool(qdict, "cpustate_all", false);
> +CPUState *cs;
>
> -if (!cs) {
> -monitor_printf(mon, "No CPU available\n");
> -return;
> +if (all_cpus) {
> +CPU_FOREACH(cs) {
Could we add a monitor_printf here giving the CPU number ?
It would just make it a little easier when you have big list of CPUs
and each has lots of registers to be able to see which one you're
looking at.
Thanks,
Dave
> +cpu_dump_state(cs, (FILE *)mon, monitor_fprintf, CPU_DUMP_FPU);
> +}
> +} else {
> +cs = mon_get_cpu();
> +
> +if (!cs) {
> +monitor_printf(mon, "No CPU available\n");
> +return;
> +}
> +
> +cpu_dump_state(cs, (FILE *)mon, monitor_fprintf, CPU_DUMP_FPU);
> }
> -cpu_dump_state(cs, (FILE *)mon, monitor_fprintf, CPU_DUMP_FPU);
> }
>
> static void hmp_info_jit(Monitor *mon, const QDict *qdict)
> --
> 2.9.4
Dave
>
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [Qemu-devel] [PATCH 1/4] egl-helpers: add helpers to handle opengl framebuffers
Hi
On Tue, Jun 6, 2017 at 3:06 PM Gerd Hoffmann wrote:
> Add a collection of egl_fb_*() helper functions to manage and use opengl
> framebuffers, which is a common pattern in UI code with opengl support.
>
> Signed-off-by: Gerd Hoffmann
> ---
> include/ui/egl-helpers.h | 14 ++
> ui/egl-helpers.c | 69
>
> 2 files changed, 83 insertions(+)
>
> diff --git a/include/ui/egl-helpers.h b/include/ui/egl-helpers.h
> index c785d60e91..01aa45fb32 100644
> --- a/include/ui/egl-helpers.h
> +++ b/include/ui/egl-helpers.h
> @@ -8,6 +8,20 @@
> extern EGLDisplay *qemu_egl_display;
> extern EGLConfig qemu_egl_config;
>
> +typedef struct egl_fb {
> +int width;
> +int height;
> +GLuint texture;
> +GLuint framebuffer;
> +bool delete_texture;
> +} egl_fb;
> +
> +void egl_fb_destroy(egl_fb *fb);
> +void egl_fb_create_for_tex(egl_fb *fb, int width, int height, GLuint
> texture);
> +void egl_fb_create_new_tex(egl_fb *fb, int width, int height);
> +void egl_fb_blit(egl_fb *dst, egl_fb *src, bool flip);
> +void egl_fb_read(void *dst, egl_fb *src);
> +
> #ifdef CONFIG_OPENGL_DMABUF
>
> extern int qemu_egl_rn_fd;
> diff --git a/ui/egl-helpers.c b/ui/egl-helpers.c
> index 4a4d3370ee..b14250ae62 100644
> --- a/ui/egl-helpers.c
> +++ b/ui/egl-helpers.c
> @@ -24,6 +24,75 @@
> EGLDisplay *qemu_egl_display;
> EGLConfig qemu_egl_config;
>
> +/* -- */
> +
> +void egl_fb_destroy(egl_fb *fb)
> +{
> +if (!fb->framebuffer) {
> +return;
> +}
> +
> +if (fb->delete_texture) {
> +glDeleteTextures(1, &fb->texture);
> +fb->delete_texture = false;
> +}
> +glDeleteFramebuffers(1, &fb->framebuffer);
> +
> +fb->width = 0;
> +fb->height = 0;
> +fb->texture = 0;
> +fb->framebuffer = 0;
> +}
> +
> +void egl_fb_create_for_tex(egl_fb *fb, int width, int height, GLuint
> texture)
> +{
> +fb->width = width;
> +fb->height = height;
> +fb->texture = texture;
> +if (!fb->framebuffer) {
> +glGenFramebuffers(1, &fb->framebuffer);
> +}
> +
> +glBindFramebuffer(GL_FRAMEBUFFER_EXT, fb->framebuffer);
> +glFramebufferTexture2DEXT(GL_FRAMEBUFFER_EXT,
> GL_COLOR_ATTACHMENT0_EXT,
> + GL_TEXTURE_2D, fb->texture, 0);
> +}
> +
> +void egl_fb_create_new_tex(egl_fb *fb, int width, int height)
> +{
> +GLuint texture;
> +
> +glGenTextures(1, &texture);
> +glBindTexture(GL_TEXTURE_2D, texture);
> +glTexImage2D(GL_TEXTURE_2D, 0, GL_RGB, width, height,
> + 0, GL_BGRA, GL_UNSIGNED_BYTE, 0);
> +
> +egl_fb_create_for_tex(fb, width, height, texture);
> +fb->delete_texture = true;
> +}
> +
> +void egl_fb_blit(egl_fb *dst, egl_fb *src, bool flip)
> +{
> +GLuint y1, y2;
> +
> +glBindFramebuffer(GL_READ_FRAMEBUFFER, src->framebuffer);
> +glBindFramebuffer(GL_DRAW_FRAMEBUFFER, dst->framebuffer);
> +glViewport(0, 0, dst->width, dst->height);
> +y1 = flip ? src->height : 0;
> +y2 = flip ? 0 : src->height;
> +glBlitFramebuffer(0, y1, src->width, y2,
> + 0, 0, dst->width, dst->height,
> + GL_COLOR_BUFFER_BIT, GL_NEAREST);
>
Not much to say, except that GL_LINEAR could give better visual results for
desktop display when scaling down (if this case could happen).
> +}
> +
> +void egl_fb_read(void *dst, egl_fb *src)
> +{
> +glBindFramebuffer(GL_READ_FRAMEBUFFER, src->framebuffer);
> +glReadBuffer(GL_COLOR_ATTACHMENT0_EXT);
> +glReadPixels(0, 0, src->width, src->height,
> + GL_BGRA, GL_UNSIGNED_BYTE, dst);
> +}
> +
> /* --
> */
>
> #ifdef CONFIG_OPENGL_DMABUF
> --
> 2.9.3
>
>
the rest looks good to me,
Reviewed-by: Marc-André Lureau
--
Marc-André Lureau
[Qemu-devel] [PATCH] Makefile: Do not generate files if "configure" has not been run yet
When doing a "make -j10" in the vanilla QEMU source tree (without running "configure first), the Makefile currently generates two files already, qemu-version.h and qemu-options.def. This should not happen, so let's make these targets depend on config-host.mak. Also the python files can not be executed without $(PYTHON), so these scripts should depend on config-host.mak, too. Signed-off-by: Thomas Huth --- Makefile | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/Makefile b/Makefile index c830d7a..6786dc2 100644 --- a/Makefile +++ b/Makefile @@ -286,7 +286,7 @@ endif all: $(DOCS) $(TOOLS) $(HELPERS-y) recurse-all modules -qemu-version.h: FORCE +qemu-version.h: config-host.mak FORCE $(call quiet-command, \ (cd $(SRC_PATH); \ printf '#define QEMU_PKGVERSION '; \ @@ -312,6 +312,7 @@ qemu-version.h: FORCE config-host.h: config-host.h-timestamp config-host.h-timestamp: config-host.mak +qemu-options.def: config-host.mak qemu-options.def: $(SRC_PATH)/qemu-options.hx $(SRC_PATH)/scripts/hxtool $(call quiet-command,sh $(SRC_PATH)/scripts/hxtool -h < $< > $@,"GEN","$@") @@ -393,6 +394,8 @@ gen-out-type = $(subst .,-,$(suffix $@)) qapi-py = $(SRC_PATH)/scripts/qapi.py $(SRC_PATH)/scripts/ordereddict.py +$(qapi-py): config-host.mak + qga/qapi-generated/qga-qapi-types.c qga/qapi-generated/qga-qapi-types.h :\ $(SRC_PATH)/qga/qapi-schema.json $(SRC_PATH)/scripts/qapi-types.py $(qapi-py) $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-types.py \ -- 1.8.3.1
[Qemu-devel] [PATCH RESEND] mttcg/i386: Patch instruction using async_safe_* framework
In mttcg, calling pause_all_vcpus() during execution from the
generated TBs causes a deadlock if some vCPU is waiting for exclusive
execution in start_exclusive(). Fix this by using the aync_safe_*
framework instead of pausing vcpus for patching instructions.
CC: Paolo Bonzini
CC: Peter Maydell
Reviewed-by: Richard Henderson
Reviewed-by: Alex Bennée
Signed-off-by: Pranith Kumar
---
hw/i386/kvmvapic.c | 82 ++
1 file changed, 52 insertions(+), 30 deletions(-)
diff --git a/hw/i386/kvmvapic.c b/hw/i386/kvmvapic.c
index 82a49556af..11b0d494eb 100644
--- a/hw/i386/kvmvapic.c
+++ b/hw/i386/kvmvapic.c
@@ -383,8 +383,7 @@ static void patch_byte(X86CPU *cpu, target_ulong addr,
uint8_t byte)
cpu_memory_rw_debug(CPU(cpu), addr, &byte, 1, 1);
}
-static void patch_call(VAPICROMState *s, X86CPU *cpu, target_ulong ip,
- uint32_t target)
+static void patch_call(X86CPU *cpu, target_ulong ip, uint32_t target)
{
uint32_t offset;
@@ -393,23 +392,24 @@ static void patch_call(VAPICROMState *s, X86CPU *cpu,
target_ulong ip,
cpu_memory_rw_debug(CPU(cpu), ip + 1, (void *)&offset, sizeof(offset), 1);
}
-static void patch_instruction(VAPICROMState *s, X86CPU *cpu, target_ulong ip)
+struct PatchInfo {
+VAPICHandlers *handler;
+target_ulong ip;
+};
+
+static void do_patch_instruction(CPUState *cs, run_on_cpu_data data)
{
-CPUState *cs = CPU(cpu);
-CPUX86State *env = &cpu->env;
-VAPICHandlers *handlers;
+X86CPU *x86_cpu = X86_CPU(cs);
+CPUX86State *env = &x86_cpu->env;
+struct PatchInfo *info = (struct PatchInfo *) data.host_ptr;
+VAPICHandlers *handlers = info->handler;
+target_ulong ip = info->ip;
uint8_t opcode[2];
uint32_t imm32 = 0;
target_ulong current_pc = 0;
target_ulong current_cs_base = 0;
uint32_t current_flags = 0;
-if (smp_cpus == 1) {
-handlers = &s->rom_state.up;
-} else {
-handlers = &s->rom_state.mp;
-}
-
if (!kvm_enabled()) {
cpu_get_tb_cpu_state(env, ¤t_pc, ¤t_cs_base,
¤t_flags);
@@ -421,48 +421,70 @@ static void patch_instruction(VAPICROMState *s, X86CPU
*cpu, target_ulong ip)
}
}
-pause_all_vcpus();
-
cpu_memory_rw_debug(cs, ip, opcode, sizeof(opcode), 0);
switch (opcode[0]) {
case 0x89: /* mov r32 to r/m32 */
-patch_byte(cpu, ip, 0x50 + modrm_reg(opcode[1])); /* push reg */
-patch_call(s, cpu, ip + 1, handlers->set_tpr);
+patch_byte(x86_cpu, ip, 0x50 + modrm_reg(opcode[1])); /* push reg */
+patch_call(x86_cpu, ip + 1, handlers->set_tpr);
break;
case 0x8b: /* mov r/m32 to r32 */
-patch_byte(cpu, ip, 0x90);
-patch_call(s, cpu, ip + 1, handlers->get_tpr[modrm_reg(opcode[1])]);
+patch_byte(x86_cpu, ip, 0x90);
+patch_call(x86_cpu, ip + 1, handlers->get_tpr[modrm_reg(opcode[1])]);
break;
case 0xa1: /* mov abs to eax */
-patch_call(s, cpu, ip, handlers->get_tpr[0]);
+patch_call(x86_cpu, ip, handlers->get_tpr[0]);
break;
case 0xa3: /* mov eax to abs */
-patch_call(s, cpu, ip, handlers->set_tpr_eax);
+patch_call(x86_cpu, ip, handlers->set_tpr_eax);
break;
case 0xc7: /* mov imm32, r/m32 (c7/0) */
-patch_byte(cpu, ip, 0x68); /* push imm32 */
+patch_byte(x86_cpu, ip, 0x68); /* push imm32 */
cpu_memory_rw_debug(cs, ip + 6, (void *)&imm32, sizeof(imm32), 0);
cpu_memory_rw_debug(cs, ip + 1, (void *)&imm32, sizeof(imm32), 1);
-patch_call(s, cpu, ip + 5, handlers->set_tpr);
+patch_call(x86_cpu, ip + 5, handlers->set_tpr);
break;
case 0xff: /* push r/m32 */
-patch_byte(cpu, ip, 0x50); /* push eax */
-patch_call(s, cpu, ip + 1, handlers->get_tpr_stack);
+patch_byte(x86_cpu, ip, 0x50); /* push eax */
+patch_call(x86_cpu, ip + 1, handlers->get_tpr_stack);
break;
default:
abort();
}
-resume_all_vcpus();
+g_free(info);
+}
+
+static void patch_instruction(VAPICROMState *s, X86CPU *cpu, target_ulong ip)
+{
+CPUState *cs = CPU(cpu);
+VAPICHandlers *handlers;
+struct PatchInfo *info;
+
+if (smp_cpus == 1) {
+handlers = &s->rom_state.up;
+} else {
+handlers = &s->rom_state.mp;
+}
+
+info = g_new(struct PatchInfo, 1);
+info->handler = handlers;
+info->ip = ip;
if (!kvm_enabled()) {
-/* Both tb_lock and iothread_mutex will be reset when
- * longjmps back into the cpu_exec loop. */
-tb_lock();
-tb_gen_code(cs, current_pc, current_cs_base, current_flags, 1);
-cpu_loop_exit_noexc(cs);
+const run_on_cpu_func fn = do_patch_instruction;
+
+async_safe_run_on_cpu(cs, fn, RUN_ON_CPU_HOST_PTR(info));
+cs->exception_index = EXCP_INTERRUPT;
+cpu_loop_exit(cs);
Re: [Qemu-devel] [PATCH] vhost-user-bridge: fix iov_restore_front() warning
Hi, Michael, could you pick this patch?
thanks
On Fri, Jun 2, 2017 at 12:26 PM Marc-André Lureau <
marcandre.lur...@redhat.com> wrote:
> CC tests/vhost-user-bridge.o
> /home/dgilbert/git/qemu-world3/tests/vhost-user-bridge.c:228:23: warning:
> variables 'front' and 'iov' used in loop condition not modified in loop
> body [-Wfor-loop-analysis]
> for (cur = front; front != iov; cur++) {
> ^~~~
> 1 warning generated.
>
> Fix the loop, document the function, and fix some related assert().
>
> In practice, the loop bug was harmless because the front sg buffer is
> enough to discard/restore the header size.
>
> Reported-by: Dr. David Alan Gilbert
> Signed-off-by: Marc-André Lureau
> ---
> tests/vhost-user-bridge.c | 11 +--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/tests/vhost-user-bridge.c b/tests/vhost-user-bridge.c
> index 8618c20d53..1e5b5ca3da 100644
> --- a/tests/vhost-user-bridge.c
> +++ b/tests/vhost-user-bridge.c
> @@ -220,12 +220,18 @@ vubr_handle_tx(VuDev *dev, int qidx)
> free(elem);
> }
>
> +
> +/* this function reverse the effect of iov_discard_front() it must be
> + * called with 'front' being the original struct iovec and 'bytes'
> + * being the number of bytes you shaved off
> + */
> static void
> iov_restore_front(struct iovec *front, struct iovec *iov, size_t bytes)
> {
> struct iovec *cur;
>
> -for (cur = front; front != iov; cur++) {
> +for (cur = front; cur != iov; cur++) {
> +assert(bytes >= cur->iov_len);
> bytes -= cur->iov_len;
> }
>
> @@ -302,7 +308,8 @@ vubr_backend_recv_cb(int sock, void *ctx)
> }
> iov_from_buf(sg, elem->in_num, 0, &hdr, sizeof hdr);
> total += hdrlen;
> -assert(iov_discard_front(&sg, &num, hdrlen) == hdrlen);
> +ret = iov_discard_front(&sg, &num, hdrlen);
> +assert(ret == hdrlen);
> }
>
> struct msghdr msg = {
> --
> 2.13.0.91.g00982b8dd
>
>
> --
Marc-André Lureau
Re: [Qemu-devel] [PULL 18/33] vhost-user-scsi: Introduce vhost-user-scsi host device
> > This could be documented, > > It is documented AFAIK. Pls take a look at the spec documentation. Found it now. It's not under GET_VRING_BASE, it's under "starting and stopping rings"---fair enough. In the case of vhost-user-scsi, however, QEMU also must not proceed until vhost-user-scsi has drained the pending I/O---and this pending I/O would be completed _after_ QEMU has sent GET_VRING_BASE. Is this handled by VHOST_USER_PROTOCOL_F_REPLY_ACK already? If so, migration would be denied if the server lacks that protocol feature. Paolo > > but perhaps it's best to add a START_STOP > > feature and message to the vhost-user protocol? > > We just never need to GET_VRING_BASE if ring keeps going - > makes no sense since base gets invalidated immediately. > > > > > The feature then can be optional for vhost-user-net and mandatory for > > vhost-user-scsi. When this is done we can remove .unmigratable. > > > > Thanks, > > > > Paolo > > If vhost-user-scsi does not stop the ring after responding to > GET_VRING_BASE, it's just a bug that needs to be fixed. > > > > > > > > > Can you please send a version of your patch that uses .unmigratable? > > > > > > Sure I can do that. We can work on the migration later on. > > > > > > > > > > > I'll send a v6 that momentarily drops vhost-scsi, but I intend to > > > > include it again in the next pull request. > > > > > > Sounds good to me. > > > > > > Felipe > > > > > > > > > > > Thanks, > > > > > > > > Paolo > > > > > > >
[Qemu-devel] [PULL 0/1] Tracing patches
The following changes since commit 0db1851becbefe3e50cfc03776fb1f75817376af: Merge remote-tracking branch 'remotes/vivier/tags/m68k-for-2.10-pull-request' into staging (2017-06-07 11:56:00 +0100) are available in the git repository at: git://github.com/stefanha/qemu.git tags/tracing-pull-request for you to fetch changes up to 249e9f792c4c6e52058570e83b550ec8310f621e: simpletrace: Improve the error message if event is not declared (2017-06-07 14:34:19 +0100) Jose Ricardo Ziviani (1): simpletrace: Improve the error message if event is not declared scripts/simpletrace.py | 10 +- 1 file changed, 9 insertions(+), 1 deletion(-) -- 2.9.4
[Qemu-devel] [PULL for-2.9 1/1] simpletrace: Improve the error message if event is not declared
From: Jose Ricardo Ziviani
Today, if we use a trace-event file which does not declare an event
existing in the log file we'll get the following error:
$ scripts/simpletrace.py trace-events trace-68508
Traceback (most recent call last):
File "scripts/simpletrace.py", line 242, in
run(Formatter())
File "scripts/simpletrace.py", line 217, in run
process(events, sys.argv[2], analyzer, read_header=read_header)
File "scripts/simpletrace.py", line 192, in process
for rec in read_trace_records(edict, log):
File "scripts/simpletrace.py", line 107, in read_trace_records
rec = read_record(edict, idtoname, fobj)
File "scripts/simpletrace.py", line 71, in read_record
return get_record(edict, idtoname, rechdr, fobj)
File "scripts/simpletrace.py", line 45, in get_record
event = edict[name]
KeyError: 'qemu_mutex_locked'
This patch improves this error by adding a hint instead of just that
KeyError log:
$ scripts/simpletrace.py trace-events trace-68508
'qemu_mutex_locked' event is logged but is not declared in the trace
events file, try using trace-events-all instead.
Signed-off-by: Jose Ricardo Ziviani
Reviewed-by: Philippe Mathieu-Daudé
Message-id: 1496075404-8845-1-git-send-email-jos...@linux.vnet.ibm.com
Signed-off-by: Stefan Hajnoczi
---
scripts/simpletrace.py | 10 +-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/scripts/simpletrace.py b/scripts/simpletrace.py
index d60b3a0..f1be6e4 100755
--- a/scripts/simpletrace.py
+++ b/scripts/simpletrace.py
@@ -42,7 +42,15 @@ def get_record(edict, idtoname, rechdr, fobj):
event_id = rechdr[0]
name = idtoname[event_id]
rec = (name, rechdr[1], rechdr[3])
-event = edict[name]
+try:
+event = edict[name]
+except KeyError, e:
+import sys
+sys.stderr.write('%s event is logged but is not declared ' \
+ 'in the trace events file, try using ' \
+ 'trace-events-all instead.\n' % str(e))
+sys.exit(1)
+
for type, name in event.args:
if is_string(type):
l = fobj.read(4)
--
2.9.4
[Qemu-devel] [PULL 1/1] simpletrace: Improve the error message if event is not declared
From: Jose Ricardo Ziviani
Today, if we use a trace-event file which does not declare an event
existing in the log file we'll get the following error:
$ scripts/simpletrace.py trace-events trace-68508
Traceback (most recent call last):
File "scripts/simpletrace.py", line 242, in
run(Formatter())
File "scripts/simpletrace.py", line 217, in run
process(events, sys.argv[2], analyzer, read_header=read_header)
File "scripts/simpletrace.py", line 192, in process
for rec in read_trace_records(edict, log):
File "scripts/simpletrace.py", line 107, in read_trace_records
rec = read_record(edict, idtoname, fobj)
File "scripts/simpletrace.py", line 71, in read_record
return get_record(edict, idtoname, rechdr, fobj)
File "scripts/simpletrace.py", line 45, in get_record
event = edict[name]
KeyError: 'qemu_mutex_locked'
This patch improves this error by adding a hint instead of just that
KeyError log:
$ scripts/simpletrace.py trace-events trace-68508
'qemu_mutex_locked' event is logged but is not declared in the trace
events file, try using trace-events-all instead.
Signed-off-by: Jose Ricardo Ziviani
Reviewed-by: Philippe Mathieu-Daudé
Message-id: 1496075404-8845-1-git-send-email-jos...@linux.vnet.ibm.com
Signed-off-by: Stefan Hajnoczi
---
scripts/simpletrace.py | 10 +-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/scripts/simpletrace.py b/scripts/simpletrace.py
index d60b3a0..f1be6e4 100755
--- a/scripts/simpletrace.py
+++ b/scripts/simpletrace.py
@@ -42,7 +42,15 @@ def get_record(edict, idtoname, rechdr, fobj):
event_id = rechdr[0]
name = idtoname[event_id]
rec = (name, rechdr[1], rechdr[3])
-event = edict[name]
+try:
+event = edict[name]
+except KeyError, e:
+import sys
+sys.stderr.write('%s event is logged but is not declared ' \
+ 'in the trace events file, try using ' \
+ 'trace-events-all instead.\n' % str(e))
+sys.exit(1)
+
for type, name in event.args:
if is_string(type):
l = fobj.read(4)
--
2.9.4
[Qemu-devel] [PULL for-2.9 0/1] Tracing patches
The following changes since commit 0db1851becbefe3e50cfc03776fb1f75817376af: Merge remote-tracking branch 'remotes/vivier/tags/m68k-for-2.10-pull-request' into staging (2017-06-07 11:56:00 +0100) are available in the git repository at: git://github.com/stefanha/qemu.git tags/tracing-pull-request for you to fetch changes up to 249e9f792c4c6e52058570e83b550ec8310f621e: simpletrace: Improve the error message if event is not declared (2017-06-07 14:34:19 +0100) Jose Ricardo Ziviani (1): simpletrace: Improve the error message if event is not declared scripts/simpletrace.py | 10 +- 1 file changed, 9 insertions(+), 1 deletion(-) -- 2.9.4
[Qemu-devel] [PATCH 3/3] test-char: start a /char/serial test
Quite limited test, to check that the chardev can be created with a
path and with the tty alias.
Signed-off-by: Marc-André Lureau
---
tests/test-char.c | 30 ++
1 file changed, 30 insertions(+)
diff --git a/tests/test-char.c b/tests/test-char.c
index dfe856cb85..ecc3fec194 100644
--- a/tests/test-char.c
+++ b/tests/test-char.c
@@ -5,6 +5,7 @@
#include "qemu/config-file.h"
#include "qemu/sockets.h"
#include "chardev/char-fe.h"
+#include "chardev/char-serial.h" /* for HAVE_CHARDEV_SERIAL */
#include "sysemu/sysemu.h"
#include "qapi/error.h"
#include "qom/qom-qobject.h"
@@ -450,6 +451,32 @@ static void char_udp_test(void)
g_free(tmp);
}
+#ifdef HAVE_CHARDEV_SERIAL
+static void char_serial_test(void)
+{
+QemuOpts *opts;
+Chardev *chr;
+
+opts = qemu_opts_create(qemu_find_opts("chardev"), "serial-id",
+1, &error_abort);
+qemu_opt_set(opts, "backend", "serial", &error_abort);
+qemu_opt_set(opts, "path", "/dev/null", &error_abort);
+
+chr = qemu_chr_new_from_opts(opts, NULL);
+g_assert_nonnull(chr);
+/* TODO: add more tests with a pty */
+object_unparent(OBJECT(chr));
+
+/* test tty alias */
+qemu_opt_set(opts, "backend", "tty", &error_abort);
+chr = qemu_chr_new_from_opts(opts, NULL);
+g_assert_nonnull(chr);
+object_unparent(OBJECT(chr));
+
+qemu_opts_del(opts);
+}
+#endif
+
static void char_file_test(void)
{
char *tmp_path = g_dir_make_tmp("qemu-test-char.XX", NULL);
@@ -597,6 +624,9 @@ int main(int argc, char **argv)
g_test_add_func("/char/file", char_file_test);
g_test_add_func("/char/socket", char_socket_test);
g_test_add_func("/char/udp", char_udp_test);
+#ifdef HAVE_CHARDEV_SERIAL
+g_test_add_func("/char/serial", char_serial_test);
+#endif
return g_test_run();
}
--
2.13.0.91.g00982b8dd
[Qemu-devel] [PATCH 2/3] chardev: don't use alias names in parse_compat()
"parport" is considered "old" since commit 88a946d32d, when "parallel"
was added. Similarly for "tty" in commit d59044ef74d.
Signed-off-by: Marc-André Lureau
---
chardev/char.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/chardev/char.c b/chardev/char.c
index f38fac5c6b..55b671973b 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -452,12 +452,12 @@ QemuOpts *qemu_chr_parse_compat(const char *label, const
char *filename)
}
if (strstart(filename, "/dev/parport", NULL) ||
strstart(filename, "/dev/ppi", NULL)) {
-qemu_opt_set(opts, "backend", "parport", &error_abort);
+qemu_opt_set(opts, "backend", "parallel", &error_abort);
qemu_opt_set(opts, "path", filename, &error_abort);
return opts;
}
if (strstart(filename, "/dev/", NULL)) {
-qemu_opt_set(opts, "backend", "tty", &error_abort);
+qemu_opt_set(opts, "backend", "serial", &error_abort);
qemu_opt_set(opts, "path", filename, &error_abort);
return opts;
}
--
2.13.0.91.g00982b8dd
[Qemu-devel] [PATCH 1/3] char: fix alias devices regression
Fix regression from commit 4d43a603c71, where the serial and parallel headers got removed from char.c, which broke the alias table. Signed-off-by: Marc-André Lureau --- chardev/char.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/chardev/char.c b/chardev/char.c index 7aa0210765..f38fac5c6b 100644 --- a/chardev/char.c +++ b/chardev/char.c @@ -34,6 +34,8 @@ #include "qemu/help_option.h" #include "chardev/char-mux.h" +#include "chardev/char-parallel.h" /* for HAVE_CHARDEV_PARPORT */ +#include "chardev/char-serial.h" /* for HAVE_CHARDEV_SERIAL */ /***/ /* character device */ -- 2.13.0.91.g00982b8dd
