Re: [Qemu-devel] [PATCH] qxl: Export "mode" as a property
On 11/19/12 14:11, Alexander Larsson wrote: > This way you can easily tell from qemu if the guest is using > a qxl driver or not. Device properties are for configuration, not inspection. cheers, Gerd
[Qemu-devel] [PATCH 0/4] AER-KVM: Error containment of PCI pass-thru devices assigned to KVM guests
Add support for error containment when a PCI pass-thru device assigned to a KVM guest encounters an error. This is for PCIe devices/drivers that support AER functionality. When the OS is notified of an error in a device either through the firmware first approach or through an interrupt handled by the AER root port driver, concerned subsystems are notified by invoking callbacks registered by these subsystems. The device is also marked as tainted till the corresponding driver recovery routines are successful. KVM module registers for a notification of such errors. In the KVM callback routine, a global counter is incremented to keep track of the error notification. Before each CPU enters guest mode to execute guest code, appropriate checks are done to see if the impacted device belongs to the guest or not. If the device belongs to the guest, qemu hypervisor for the guest is informed and the guest is immediately brought down, thus preventing or minimizing chances of any bad data being written out by the guest driver after the device has encountered an error. Note that the changes here are specific to PCI pass-thru devices and is confined to error containment. Error recovery is not included in these set of changes. A future set of patches is planned to address SR-IOV devices and VFIO devices assigned to guests as well as recovery without bringing down the guest. --- Vijay Mohan Pandarathil(4): AER-PCI: Add infrastructure for notification of errors to other subsystems AER-GHES: Add support for error notification in firmware first approach of AER AER-KVM: Integration of KVM with AER for PCI pass-thru devices AER-QEMU: Bring down the guest when KVM detects a PCI device error arch/x86/include/asm/kvm_host.h| 1 + arch/x86/kvm/x86.c | 44 ++ drivers/acpi/apei/ghes.c | 41 +++ drivers/pci/pcie/aer/aerdrv.c | 20 + drivers/pci/pcie/aer/aerdrv_core.c | 9 +++- include/linux/aer.h| 4 include/linux/kvm_host.h | 4 include/linux/pci.h| 2 ++ include/uapi/linux/kvm.h | 1 + virt/kvm/assigned-dev.c| 34 + virt/kvm/kvm_main.c| 34 + 11 files changed, 193 insertions(+), 1 deletion(-) Qemu files changed kvm-all.c |6 ++ linux-headers/linux/kvm.h |1 + 2 files changed, 7 insertions(+)
[Qemu-devel] [PATCH 4/4] AER-QEMU: Bring down the guest when KVM detects a PCI device error
- When KVM_RUN ioctl returns with an exit reason requesting a shutdown
of the guest due to a PCI device error detected by AER, shutdown the
guest immediately.
Signed-off-by: Vijay Mohan Pandarathil
---
kvm-all.c | 6 ++
linux-headers/linux/kvm.h | 1 +
2 files changed, 7 insertions(+)
diff --git a/kvm-all.c b/kvm-all.c
index b6d0483..aaff44c 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -1592,6 +1592,12 @@ int kvm_cpu_exec(CPUArchState *env)
qemu_system_reset_request();
ret = EXCP_INTERRUPT;
break;
+case KVM_EXIT_AER_SHUTDOWN:
+fprintf(stderr, "KVM: PCI device assigned to guest encountered "
+ "an uncorrectable error. Stopping guest\n");
+qemu_system_shutdown_request();
+ret = EXCP_INTERRUPT;
+break;
case KVM_EXIT_UNKNOWN:
fprintf(stderr, "KVM: unknown exit, hardware reason %" PRIx64 "\n",
(uint64_t)run->hw.hardware_exit_reason);
diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
index 81d2feb..64906ef 100644
--- a/linux-headers/linux/kvm.h
+++ b/linux-headers/linux/kvm.h
@@ -167,6 +167,7 @@ struct kvm_pit_config {
#define KVM_EXIT_OSI 18
#define KVM_EXIT_PAPR_HCALL 19
#define KVM_EXIT_S390_UCONTROL 20
+#define KVM_EXIT_AER_SHUTDOWN 21
/* For KVM_EXIT_INTERNAL_ERROR */
#define KVM_INTERNAL_ERROR_EMULATION 1
--
1.7.11.3
Re: [Qemu-devel] [PATCH] target-mips: Add comments on POOL32Axf encoding
ping?
On Fri, Nov 16, 2012 at 10:29:47AM +0800, 陳韋任 (Wei-Ren Chen) wrote:
> Hi all,
>
> Current QEMU MIPS POOL32AXF encoding comes from microMIPS32
> and microMIPS32 DSP. Add comment here to help reading.
>
> Please review, thanks.
>
> Regards,
> chenwj
>
> Signed-off-by: Chen Wei-Ren
> ---
> target-mips/translate.c | 17 +
> 1 files changed, 17 insertions(+), 0 deletions(-)
>
> diff --git a/target-mips/translate.c b/target-mips/translate.c
> index 01b48fa..9d4b2c3 100644
> --- a/target-mips/translate.c
> +++ b/target-mips/translate.c
> @@ -10359,6 +10359,19 @@ enum {
>
> /* POOL32AXF encoding of minor opcode field extension */
>
> +/*
> + * 1. MIPS Architecture for Programmers Volume II-B:
> + * The microMIPS32 Instruction Set (Revision 3.05)
> + *
> + * Table 6.5 POOL32Axf Encoding of Minor Opcode Extension Field
> + *
> + * 2. MIPS Architecture for Programmers VolumeIV-e:
> + * The MIPS DSP Application-Specific Extension
> + * to the microMIPS32 Architecture (Revision 2.34)
> + *
> + * Table 5.5 POOL32Axf Encoding of Minor Opcode Extension Field
> + */
> +
> enum {
> /* bits 11..6 */
> TEQ = 0x00,
> @@ -10371,6 +10384,8 @@ enum {
> MFC0 = 0x03,
> MTC0 = 0x0b,
>
> +/* begin of microMIPS32 DSP */
> +
> /* bits 13..12 for 0x01 */
> MFHI_ACC = 0x0,
> MFLO_ACC = 0x1,
> @@ -10387,6 +10402,8 @@ enum {
> MULT_ACC = 0x0,
> MULTU_ACC = 0x1,
>
> +/* end of microMIPS32 DSP */
> +
> /* bits 15..12 for 0x2c */
> SEB = 0x2,
> SEH = 0x3,
> --
> 1.7.3.4
--
Wei-Ren Chen (陳韋任)
Computer Systems Lab, Institute of Information Science,
Academia Sinica, Taiwan (R.O.C.)
Tel:886-2-2788-3799 #1667
Homepage: http://people.cs.nctu.edu.tw/~chenwj
[Qemu-devel] [PATCH 3/4] AER-KVM: Integration of KVM with AER for PCI pass-thru devices
- Register a notifier function to be called whenever a PCIe error is
detected by the AER subsystem.
- The notifier function bumps up a global count to keep track of the
error notifications.
- Before guest entry, each vcpu checks if there has been any new
notifications since last check. If any, check if the device impacted
is assigned to the guest. If impacted, return to qemu requesting that
the guest be brought down. If no device assigned to the guest is impacted,
sync up the per guest notified count to the global value.
- At guest start time, check if any of the PCI devices assigned to the
guest is faulty and if so, fail the guest startup.
Signed-off-by: Vijay Mohan Pandarathil
---
arch/x86/include/asm/kvm_host.h | 1 +
arch/x86/kvm/x86.c | 44 +
include/linux/kvm_host.h| 4
include/uapi/linux/kvm.h| 1 +
virt/kvm/assigned-dev.c | 34 +++
virt/kvm/kvm_main.c | 34 +++
6 files changed, 118 insertions(+)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index b2e11f4..481ad94 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -951,6 +951,7 @@ enum {
*/
asmlinkage void kvm_spurious_fault(void);
extern bool kvm_rebooting;
+extern unsigned long kvm_aer_notified_cnt;
#define kvm_handle_fault_on_reboot(insn, cleanup_insn) \
"666: " insn "\n\t" \
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 4f76417..87e3c3e 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -5235,6 +5235,32 @@ static void process_nmi(struct kvm_vcpu *vcpu)
kvm_make_request(KVM_REQ_EVENT, vcpu);
}
+/*
+ * This function checks if KVM has been notified of any PCI error since last
+ * checked by this guest. If so, it checks if any PCI device assigned to this
+ * guest has got the error. If not, adjust the per guest notified_cnt to match
+ * the global kvm notified_cnt
+ */
+static inline int kvm_aer_exit(struct kvm *kvm)
+{
+ if (kvm_aer_notified_cnt == kvm->aer_notified_cnt)
+ return 0;
+
+ /*
+* These errors are expected to be very rare. In the case
+* of an error notification, multiple vcpu threads could reach
+* here and do the device check below. However, functionally
+* it shouldn't cause a problem.
+*/
+ if (kvm_find_assigned_dev_err(kvm)) {
+ return 1;
+ } else {
+ spin_lock(&kvm->aer_lock);
+ kvm->aer_notified_cnt = kvm_aer_notified_cnt;
+ spin_unlock(&kvm->aer_lock);
+ return 0;
+ }
+}
static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
{
int r;
@@ -5334,6 +5360,24 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
goto cancel_injection;
}
+ /*
+* If any of the PCI devices assigned to a guest is reported to have
+* uncorrected error, do not allow guest code to execute, instead
+* bring down the guest to contain the error. Note that there is a
+* small window here where a new error notification could come in while
+* while the check is being done or right after the check before the cpu
+* enters the guest mode. Not sure if this check needs to be after
+* kvm_guest_enter() ?
+*/
+ if (kvm_aer_exit(vcpu->kvm)) {
+ vcpu->mode = OUTSIDE_GUEST_MODE;
+ smp_wmb();
+ local_irq_enable();
+ preempt_enable();
+ r = 0;
+ vcpu->run->exit_reason = KVM_EXIT_AER_SHUTDOWN;
+ goto cancel_injection;
+ }
srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
if (req_immediate_exit)
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index ecc5543..b3c2730 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -364,6 +364,8 @@ struct kvm {
long mmu_notifier_count;
#endif
long tlbs_dirty;
+ spinlock_t aer_lock;
+ unsigned long aer_notified_cnt;
};
#define kvm_err(fmt, ...) \
@@ -933,6 +935,8 @@ static inline long kvm_vm_ioctl_assigned_device(struct kvm
*kvm, unsigned ioctl,
#endif
+int kvm_find_assigned_dev_err(struct kvm *kvm);
+
static inline void kvm_make_request(int req, struct kvm_vcpu *vcpu)
{
set_bit(req, &vcpu->requests);
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 0a6d6ba..6263c21 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -167,6 +167,7 @@ struct kvm_pit_config {
#define KVM_EXIT_OSI 18
#define KVM_EXIT_PAPR_HCALL 19
#define KVM_EXIT_S390_UCONTROL 20
+#define KVM_EXIT_AER_SHUTDOWN 21
/* For KVM_EXIT_INTERNAL_ERROR */
#define KVM_INTERNAL_ERROR_EMULATION 1
diff --git a/virt/kvm/assigned-dev.c
[Qemu-devel] [PATCH 2/4] AER-GHES: Add support for error notification in firmware first approach of AER
- When an uncorrected recoverable error is reported via an NMI in the
firmware first model of AER, invoke the callbacks registered for
notifications as well as mark the device as tainted.
- Add a new function ghes_mark_dev_err() leveraged from ghes_do_proc()
to identify the device from the error record and mark it as tainted.
Signed-off-by: Vijay Mohan Pandarathil
---
drivers/acpi/apei/ghes.c | 41 +
1 file changed, 41 insertions(+)
diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 1599566..7b077a7 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -502,6 +502,45 @@ static void ghes_do_proc(const struct
acpi_hest_generic_status *estatus)
}
}
+/*
+ * If an uncorrected recoverable PCIe error is reported, the corresponding
+ * PCI device is marked as tainted. The device remains tainted until the
+ * claiming driver does a recovery. The PCI device is identified from the
+ * error record.
+ */
+static void ghes_mark_dev_err(const struct acpi_hest_generic_status *estatus)
+{
+ int sev, sec_sev;
+ struct acpi_hest_generic_data *gdata;
+
+ sev = ghes_severity(estatus->error_severity);
+ apei_estatus_for_each_section(estatus, gdata) {
+ sec_sev = ghes_severity(gdata->error_severity);
+ if (!uuid_le_cmp(*(uuid_le *)gdata->section_type,
+ CPER_SEC_PCIE)) {
+ struct cper_sec_pcie *pcie_err;
+ pcie_err = (struct cper_sec_pcie *)(gdata+1);
+ if (sev == GHES_SEV_RECOVERABLE &&
+ sec_sev == GHES_SEV_RECOVERABLE &&
+ pcie_err->validation_bits &
+ CPER_PCIE_VALID_DEVICE_ID &&
+ pcie_err->validation_bits &
+ CPER_PCIE_VALID_AER_INFO) {
+ unsigned int devfn;
+ struct pci_dev *pdev;
+ devfn = PCI_DEVFN(pcie_err->device_id.device,
+ pcie_err->device_id.function);
+ pdev = pci_get_domain_bus_and_slot(
+ pcie_err->device_id.segment,
+ pcie_err->device_id.bus,
+ devfn);
+ if (!pdev)
+ continue;
+ pdev->dev_flags |= PCI_DEV_FLAGS_ERR_DETECTED;
+ }
+ }
+ }
+}
static void __ghes_print_estatus(const char *pfx,
const struct acpi_hest_generic *generic,
const struct acpi_hest_generic_status *estatus)
@@ -868,6 +907,8 @@ static int ghes_notify_nmi(unsigned int cmd, struct pt_regs
*regs)
estatus = GHES_ESTATUS_FROM_NODE(estatus_node);
memcpy(estatus, ghes->estatus, len);
llist_add(&estatus_node->llnode, &ghes_estatus_llist);
+ ghes_mark_dev_err(estatus);
+ aer_notify(0, NULL);
}
next:
#endif
--
1.7.11.3
[Qemu-devel] [PATCH 1/4] AER-PCI: Add infrastructure for notification of errors to other subsystems
- Adds infrastructure support for error notifications from AER subsystem
to other subsystems. The generic notifier_chain functionality is used.
- When the AER rootport driver detects an uncorrected error, invoke the
callbacks registered for notifications as well as mark the device as
tainted.
- After the recovery is successful, clear the tainted flag on the device.
Signed-off-by: Vijay Mohan Pandarathil
---
drivers/pci/pcie/aer/aerdrv.c | 20
drivers/pci/pcie/aer/aerdrv_core.c | 9 -
include/linux/aer.h| 4
include/linux/pci.h| 2 ++
4 files changed, 34 insertions(+), 1 deletion(-)
diff --git a/drivers/pci/pcie/aer/aerdrv.c b/drivers/pci/pcie/aer/aerdrv.c
index 030cf12..92dc54c 100644
--- a/drivers/pci/pcie/aer/aerdrv.c
+++ b/drivers/pci/pcie/aer/aerdrv.c
@@ -68,6 +68,26 @@ static struct pcie_port_service_driver aerdriver = {
static int pcie_aer_disable;
+ATOMIC_NOTIFIER_HEAD(aer_notifier_list);
+
+void aer_notifier_register(struct notifier_block *nb)
+{
+ atomic_notifier_chain_register(&aer_notifier_list, nb);
+}
+EXPORT_SYMBOL_GPL(aer_notifier_register);
+
+void aer_notifier_unregister(struct notifier_block *nb)
+{
+ atomic_notifier_chain_unregister(&aer_notifier_list, nb);
+}
+EXPORT_SYMBOL_GPL(aer_notifier_unregister);
+
+void aer_notify(unsigned long val, void *v)
+{
+ atomic_notifier_call_chain(&aer_notifier_list, val, v);
+}
+EXPORT_SYMBOL_GPL(aer_notify);
+
void pci_no_aer(void)
{
pcie_aer_disable = 1; /* has priority over 'forceload' */
diff --git a/drivers/pci/pcie/aer/aerdrv_core.c
b/drivers/pci/pcie/aer/aerdrv_core.c
index af4e31c..be6c3ee 100644
--- a/drivers/pci/pcie/aer/aerdrv_core.c
+++ b/drivers/pci/pcie/aer/aerdrv_core.c
@@ -215,6 +215,8 @@ static int report_error_detected(struct pci_dev *dev, void
*data)
device_lock(&dev->dev);
dev->error_state = result_data->state;
+ dev->dev_flags |= PCI_DEV_FLAGS_ERR_DETECTED;
+ aer_notify(0, NULL);
if (!dev->driver ||
!dev->driver->err_handler ||
@@ -291,6 +293,7 @@ static int report_resume(struct pci_dev *dev, void *data)
device_lock(&dev->dev);
dev->error_state = pci_channel_io_normal;
+ dev->dev_flags &= ~PCI_DEV_FLAGS_ERR_DETECTED;
if (!dev->driver ||
!dev->driver->err_handler ||
@@ -521,6 +524,7 @@ static void do_recovery(struct pci_dev *dev, int severity)
"resume",
report_resume);
+ dev->dev_flags &= ~PCI_DEV_FLAGS_ERR_DETECTED;
dev_info(&dev->dev, "AER: Device recovery successful\n");
return;
@@ -552,8 +556,11 @@ static void handle_error_source(struct pcie_device *aerdev,
if (pos)
pci_write_config_dword(dev, pos + PCI_ERR_COR_STATUS,
info->status);
- } else
+ } else {
+ dev->dev_flags |= PCI_DEV_FLAGS_ERR_DETECTED;
+ aer_notify(0, NULL);
do_recovery(dev, info->severity);
+ }
}
#ifdef CONFIG_ACPI_APEI_PCIEAER
diff --git a/include/linux/aer.h b/include/linux/aer.h
index 544abdb..f8df468 100644
--- a/include/linux/aer.h
+++ b/include/linux/aer.h
@@ -54,5 +54,9 @@ extern void cper_print_aer(const char *prefix, int
cper_severity,
extern int cper_severity_to_aer(int cper_severity);
extern void aer_recover_queue(int domain, unsigned int bus, unsigned int devfn,
int severity);
+extern void aer_notifier_register(struct notifier_block *nb);
+extern void aer_notifier_unregister(struct notifier_block *nb);
+extern void aer_notify(unsigned long val, void *v);
+
#endif //_AER_H_
diff --git a/include/linux/pci.h b/include/linux/pci.h
index ee21795..ab17a08 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -155,6 +155,8 @@ enum pci_dev_flags {
PCI_DEV_FLAGS_NO_D3 = (__force pci_dev_flags_t) 2,
/* Provide indication device is assigned by a Virtual Machine Manager */
PCI_DEV_FLAGS_ASSIGNED = (__force pci_dev_flags_t) 4,
+ /* Indicates that hw has reported an uncorrected error for the device */
+ PCI_DEV_FLAGS_ERR_DETECTED = (__force pci_dev_flags_t) 8,
};
enum pci_irq_reroute_variant {
--
1.7.11.3
Re: [Qemu-devel] [PATCH] m25p80: Fix wrong jedec id for Numonyx n25q128
On Tue, Nov 20, 2012 at 02:32:33PM +1000, Peter Crosthwaite wrote:
>Hi Liming,
>
>On Mon, Nov 19, 2012 at 11:03 PM, Liming Wang wrote:
>> The jedec id of "n25q128" should be 0x20bb18, not 0x20ba18.
>>
>> Signed-off-by: Liming Wang
>> ---
>> hw/m25p80.c |2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/m25p80.c b/hw/m25p80.c
>> index 3895e73..58ae754 100644
>> --- a/hw/m25p80.c
>> +++ b/hw/m25p80.c
>> @@ -177,7 +177,7 @@ static const FlashPartInfo known_devices[] = {
>> { INFO("w25q64", 0xef4017, 0, 64 << 10, 128, ER_4K) },
>>
>> /* Numonyx -- n25q128 */
>> -{ INFO("n25q128", 0x20ba18, 0, 64 << 10, 256, 0) },
>> +{ INFO("n25q128", 0x20bb18, 0, 64 << 10, 256, 0) },
>>
>
>Im not sure this is right. Ive looked through the datasheets for this
>part. Rev 1.0 (Feb 2010) of the data sheet has the 0x20bb18 Jedec code
>but Rev 7 (Feb 2011) has 0x20ba18. We have however, noticed here with
>some actual parts that the Jedec code varies from one board to the
>next between these two. The mainline Linux Kernel uses 0x20ba18
>(drivers/mtd/devices/m25p80.c):
>
>665
>666 /* Micron */
>667 { "n25q128", INFO(0x20ba18, 0, 64 * 1024, 256, 0) },
Yes, it is. Sorry, I did't look at te mainline Linux kernel.
>668 { "n25q256a", INFO(0x20ba19, 0, 64 * 1024, 512, SECT_4K) },
>669
>
>And the Xilinx Linux kernel has both:
>
> /* Micron */
> { "n25q128", INFO(0x20ba18, 0, 64 * 1024, 256, 0) },
> { "n25q256a", INFO(0x20ba19, 0, 64 * 1024, 512, SECT_4K) },
> /* Numonyx flash n25q128 - FIXME check the name */
> { "n25q128", INFO(0x20bb18, 0, 64 * 1024, 256, 0) },
I don't know what's the Xilinx Linux kernel you used.
But from Xilinx Linux git tree, master brach, the file
"drivers/mtd/devices/m25p80.c":
http://git.xilinx.com/?p=linux-xlnx.git;a=blob;f=drivers/mtd/devices/m25p80.c;h=9d11fdeb375bb097ec2eb13497a1e67ad8babbb8;hb=refs/heads/master
has only 0x20bb18 jedec code for "n25q128":
712 /* Numonyx flash n25q128 */
713 { "n25q128", INFO(0x20bb18, 0, 64 * 1024, 256, 0) },
I looked more deeply in the n25q128 flash and I found there are
two type of "n25q128" flashes.
1. One type is with 1.8V supply voltage:
http://www.micron.com/products/nor-flash/serial-nor-flash#fullPart&236=128Mb&490=N25Q&192=1.7V-2.0V
It has prefix of "N25Q128A11". The datasheet is located in:
http://www.micron.com/~/media/Documents/Products/Data%20Sheet/NOR%20Flash/Serial%20NOR/N25Q/n25q_128mb_1_8v_65nm.pdf
In the first page, it tells that the jedec code is 0xbb18.
1. One type is with 3V supply voltage:
http://www.micron.com/products/nor-flash/serial-nor-flash#fullPart&236=128Mb&490=N25Q&192=2.7V-3.6V
It has prefix of "N25Q128A13". The datasheet is located in:
http://www.micron.com/~/media/Documents/Products/Data%20Sheet/NOR%20Flash/Serial%20NOR/N25Q/N25Q_128_3_Volt_with_boot_sector.pdf
In the first page, it tells that the jedec code is 0xba18.
So I think both 0x20bb18 and 0x20ba18 are right. It depends on what type flash
is used in the Xilinx board and what Xilinx Linux kernel you use.
For my Xilinx zc702 board, I found it has the one 1.8V flash(N25Q128A11E40) and
my
Xilinx kernel has just 0x20bb18 jedec code.
How about your Xilinx board?
And I found there is confusion in Xilinx zc702 board manual:
It declares it uses one N25Q128A13BSF40F flash, but with supply voltage 1.8V.
Qemu maybe has no need to change, but Linux kernel needs to change to
distinguish
the two type of flashes.
Regards,
Liming Wang
>
>I think 20ba18 is correct given its specifed by the more recent
>datasheets, but if you are comparing to an earlier revision Zynq board
>then you may see a diff. Also if you are using Linux, check your
>kernel to see if you have the 0x20ba10 line as older versions of the
>Xilinx kernel may only have 0x20bb18. May be a case of just a revup of
>your kernel.
>
>Another solution is to add both to QEMU, although having to
>distinguish between the two different parts with the same name is
>messy.
>
>Regards,
>Peter
>
>> { },
>> };
>> --
>> 1.7.9.5
>>
>>
Re: [Qemu-devel] [PATCH v5 5/6] i8259: fix so that dropping IRQ level always clears the interrupt request
On Mon, Nov 19, 2012 at 04:28:31PM +0100, BALATON Zoltan wrote: > On Sun, 9 Sep 2012, Matthew Ogilvie wrote: > > Intel's definition of "edge triggered" means: "asserted with a > > low-to-high transition at the time an interrupt is registered and > > then kept high until the interrupt is served via one of the > > EOI mechanisms or goes away unhandled." > > > > So the only difference between edge triggered and level triggered > > is in the leading edge, with no difference in the trailing edge. > > What happened to this patch? Any chance it will be in 1.3? > > Thanks, > BALATON Zoltan I kind of doubt it will make it into 1.3, although I would like to get it in eventually. Maybe the first few essentially unrelated trivial patches can make it in? Yours is the first comment I've seen since I've posted version 6 of the series (this particular patch is the same as version 5). The lack of feedback combined with other demands on my time have prevented me from progressing on this for over a month and a half. Status: * We can't completely fix the i8259 model without addressing some issues in the i8254 model as well. The i8254 issues (very short duty cycle on IRQ0 causing lost timer ticks) become rather severe if you try to fix the i8259 without fixing the i8254. * But the obvious direct fixes to the i8254 may risk causing issues with breaking migration between pre-fix and post-fix versions of qemu, due to lost and/or extra timer interrupts. I'm not sure how big of an issue this is in practice, but it is a concern. * I've outlined some possible ways to address the migration issue in the cover letter of version 6 of the patch series. * Similar issue cascades exist in the in-kernel KVM model as well. The i8254 issues are even more severe in KVM: timer interrupts are always lost [0-length duty cycle in KVM model], instead of sometimes lost [non-0 but tiny duty cycle in qemu model]. * I'm not aware of any similar problems in other device interrupt models, but I haven't really checked them, either. Next step?: In the absense of any other suggestions, I'm thinking about rolling a version of the series that leaves IRQ0 (timer) working the way it does in qemu 1.2. Except in the i8254 I'll immediately preceed each "intended to be leading edge transition" with an explicit lowering of IRQ0, so that if migrating from some future really-fixed version that leaves IRQ0 high most of the time, it will still recognize the new edge. A "real" fix would then be delayed (years?) until versions without this first stage fix are no longer in production. I can probably get this done this weekend. [Although I'm not sure how to deal with the issue that some of the i8254 modes' leading edge transitions are off by one count. Perhaps we'll need multiple intermediate stages, or one of the other strategies outlined in the version 6 cover letter.] Or does anyone have a better suggestion? - Matthew Ogilvie
Re: [Qemu-devel] [PATCH] m25p80: Fix wrong jedec id for Numonyx n25q128
Hi Liming,
On Mon, Nov 19, 2012 at 11:03 PM, Liming Wang wrote:
> The jedec id of "n25q128" should be 0x20bb18, not 0x20ba18.
>
> Signed-off-by: Liming Wang
> ---
> hw/m25p80.c |2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/m25p80.c b/hw/m25p80.c
> index 3895e73..58ae754 100644
> --- a/hw/m25p80.c
> +++ b/hw/m25p80.c
> @@ -177,7 +177,7 @@ static const FlashPartInfo known_devices[] = {
> { INFO("w25q64", 0xef4017, 0, 64 << 10, 128, ER_4K) },
>
> /* Numonyx -- n25q128 */
> -{ INFO("n25q128", 0x20ba18, 0, 64 << 10, 256, 0) },
> +{ INFO("n25q128", 0x20bb18, 0, 64 << 10, 256, 0) },
>
Im not sure this is right. Ive looked through the datasheets for this
part. Rev 1.0 (Feb 2010) of the data sheet has the 0x20bb18 Jedec code
but Rev 7 (Feb 2011) has 0x20ba18. We have however, noticed here with
some actual parts that the Jedec code varies from one board to the
next between these two. The mainline Linux Kernel uses 0x20ba18
(drivers/mtd/devices/m25p80.c):
665
666 /* Micron */
667 { "n25q128", INFO(0x20ba18, 0, 64 * 1024, 256, 0) },
668 { "n25q256a", INFO(0x20ba19, 0, 64 * 1024, 512, SECT_4K) },
669
And the Xilinx Linux kernel has both:
/* Micron */
{ "n25q128", INFO(0x20ba18, 0, 64 * 1024, 256, 0) },
{ "n25q256a", INFO(0x20ba19, 0, 64 * 1024, 512, SECT_4K) },
/* Numonyx flash n25q128 - FIXME check the name */
{ "n25q128", INFO(0x20bb18, 0, 64 * 1024, 256, 0) },
I think 20ba18 is correct given its specifed by the more recent
datasheets, but if you are comparing to an earlier revision Zynq board
then you may see a diff. Also if you are using Linux, check your
kernel to see if you have the 0x20ba10 line as older versions of the
Xilinx kernel may only have 0x20bb18. May be a case of just a revup of
your kernel.
Another solution is to add both to QEMU, although having to
distinguish between the two different parts with the same name is
messy.
Regards,
Peter
> { },
> };
> --
> 1.7.9.5
>
>
Re: [Qemu-devel] assertion in temp_save
On Sun, Nov 18, 2012 at 7:34 AM, Max Filippov wrote:
> On Sun, Nov 18, 2012 at 7:19 AM, Max Filippov wrote:
>> Hi Aurelien,
>>
>> starting with commit 2c0366f tcg: don't explicitly save globals and temps
>> I get the following abort on target-xtensa:
>>
>> qemu-system-xtensa: tcg/tcg.c:1665: temp_save: Assertion
>> `s->temps[temp].val_type == 2 || s->temps[temp].fixed_reg' failed.
>> Aborted
>>
>> I see that that commit only adds assertion and that bad thing happens
>> elsewhere. I've found that removal of tcg_gen_discard_i32 in the
>> gen_right_shift_sar makes it work again. The trace of the TB that fails
>> translation is below. If 'discard loc5' is removed it starts to work.
>>
>> Any idea of what might be wrong?
>
> In the debugger loc5 looks like this when abort happens:
>
> (gdb) p s->temps[105]
> $2 = {
> base_type = TCG_TYPE_I32,
> type = TCG_TYPE_I32,
> val_type = 0,
> reg = 11,
> val = 32,
> mem_reg = 4,
> mem_offset = 128,
> fixed_reg = 0,
> mem_coherent = 0,
> mem_allocated = 0,
> temp_local = 1,
> temp_allocated = 0,
> next_free_temp = -1,
> name = 0x0
> }
Looks like the issue is local temp reaching the end of TB in a dead state.
Hence the question: is discard applicable to local temps?
Or maybe I should just make it global (two other tcg values used with discard
in other targets are also globals) and avoid temp_local_new/temp_free at
first place?
--
Thanks.
-- Max
[Qemu-devel] [PATCH] kvm: fix incorrect length in a loop over kvm dirty pages map
QEMU allocates a map enough for 4k pages. However the system page size can be 64K (for example on POWER) and the host kernel uses only a small part of it as one big stores a dirty flag for 16 pages 4K each, the hpratio variable stores this ratio and the kvm_get_dirty_pages_log_range function handles it correctly. However kvm_get_dirty_pages_log_range still goes beyond the data provided by the host kernel which is not correct. It does not cause errors at the moment as the whole bitmap is zeroed before doing KVM ioctl. The patch reduces number of iterations over the map. Signed-off-by: Alexey Kardashevskiy --- kvm-all.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kvm-all.c b/kvm-all.c index b6d0483..c7f 100644 --- a/kvm-all.c +++ b/kvm-all.c @@ -364,7 +364,7 @@ static int kvm_get_dirty_pages_log_range(MemoryRegionSection *section, unsigned int i, j; unsigned long page_number, c; hwaddr addr, addr1; -unsigned int len = ((section->size / TARGET_PAGE_SIZE) + HOST_LONG_BITS - 1) / HOST_LONG_BITS; +unsigned int len = ((section->size / getpagesize()) + HOST_LONG_BITS - 1) / HOST_LONG_BITS; unsigned long hpratio = getpagesize() / TARGET_PAGE_SIZE; /* -- 1.7.10.4
[Qemu-devel] [Bug 739785] Re: qemu-i386 user mode can't fork (bash: fork: Invalid argument)
Oh yeah.. wine goes in /usr/local/bin. I created a symbolic link from /usr/gnemul/qemu-i386/usr/local/lib/libwine* /usr/local/lib. Same with the wine folder.. it needs to be seen as /usr/local/lib/wine. -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/739785 Title: qemu-i386 user mode can't fork (bash: fork: Invalid argument) Status in QEMU: New Status in “qemu” package in Debian: Confirmed Bug description: Good time of day everybody, I have been trying to make usermode qemu on ARM with plugapps (archlinux) with archlinux i386 chroot to work. 1. I installed arch linux in a virtuabox and created a chroot for it with mkarchroot. Transferred it to my pogo plug into /i386/ 2. I comiled qemu-i386 static and put it into /i386/usr/bin/ ./configure --static --disable-blobs --disable-system --target-list=i386-linux-user make 3. I also compiled linux kernel 2.6.38 with CONFIG_BINFMT_MISC=y and installed it. uname -a Linux Plugbox 2.6.38 #4 PREEMPT Fri Mar 18 22:19:10 CDT 2011 armv5tel Feroceon 88FR131 rev 1 (v5l) Marvell SheevaPlug Reference Board GNU/Linux 4. Added the following options into /etc/rc.local /sbin/modprobe binfmt_misc /bin/mount binfmt_misc -t binfmt_misc /proc/sys/fs/binfmt_misc echo ':qemu-i386:M::\x7fELF\x01\x01\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\x03\x00:\xff\xff\xff\xff\xff\xfe\xfe\xff\xff\xff\xff\xff\xff\xff\xff\xff\xfb\xff\xff\xff:/usr/bin/qemu-i386:' >/proc/sys/fs/binfmt_misc/register 5. Also copied ld-linux.so.3 (actually ld-2.13.so because ld- linux.so.3 is a link to that file) from /lib/ to /i386/lib/ 6.Now i chroot into /i386 and I get this: [root@Plugbox i386]# chroot . [II aI hnve ao n@P /]# pacman -Suy bash: fork: Invalid argument 7.I also downloaded linux-user-test-0.3 from qemu website and ran the test: [root@Plugbox linux-user-test-0.3]# make ./qemu-linux-user.sh [qemu-i386] ../qemu-0.14.0/i386-linux-user/qemu-i386 -L ./gnemul/qemu-i386 i386/ls -l dummyfile BUG IN DYNAMIC LINKER ld.so: dl-version.c: 210: _dl_check_map_versions: Assertion `needed != ((void *)0)' failed! make: *** [test] Error 127 To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/739785/+subscriptions
[Qemu-devel] [Bug 739785] Re: qemu-i386 user mode can't fork (bash: fork: Invalid argument)
I still have one program that refuses to run.. I think its having a problem with an x86 library on the gnemul side... I noticed libpng.so.3 wanted to be in the gnemul folder by wine.. even though it didn't exist on the X86 machine I was using to compile.. so I copied it from libpng12.so.. Anyone have an Idea about that??? -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/739785 Title: qemu-i386 user mode can't fork (bash: fork: Invalid argument) Status in QEMU: New Status in “qemu” package in Debian: Confirmed Bug description: Good time of day everybody, I have been trying to make usermode qemu on ARM with plugapps (archlinux) with archlinux i386 chroot to work. 1. I installed arch linux in a virtuabox and created a chroot for it with mkarchroot. Transferred it to my pogo plug into /i386/ 2. I comiled qemu-i386 static and put it into /i386/usr/bin/ ./configure --static --disable-blobs --disable-system --target-list=i386-linux-user make 3. I also compiled linux kernel 2.6.38 with CONFIG_BINFMT_MISC=y and installed it. uname -a Linux Plugbox 2.6.38 #4 PREEMPT Fri Mar 18 22:19:10 CDT 2011 armv5tel Feroceon 88FR131 rev 1 (v5l) Marvell SheevaPlug Reference Board GNU/Linux 4. Added the following options into /etc/rc.local /sbin/modprobe binfmt_misc /bin/mount binfmt_misc -t binfmt_misc /proc/sys/fs/binfmt_misc echo ':qemu-i386:M::\x7fELF\x01\x01\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\x03\x00:\xff\xff\xff\xff\xff\xfe\xfe\xff\xff\xff\xff\xff\xff\xff\xff\xff\xfb\xff\xff\xff:/usr/bin/qemu-i386:' >/proc/sys/fs/binfmt_misc/register 5. Also copied ld-linux.so.3 (actually ld-2.13.so because ld- linux.so.3 is a link to that file) from /lib/ to /i386/lib/ 6.Now i chroot into /i386 and I get this: [root@Plugbox i386]# chroot . [II aI hnve ao n@P /]# pacman -Suy bash: fork: Invalid argument 7.I also downloaded linux-user-test-0.3 from qemu website and ran the test: [root@Plugbox linux-user-test-0.3]# make ./qemu-linux-user.sh [qemu-i386] ../qemu-0.14.0/i386-linux-user/qemu-i386 -L ./gnemul/qemu-i386 i386/ls -l dummyfile BUG IN DYNAMIC LINKER ld.so: dl-version.c: 210: _dl_check_map_versions: Assertion `needed != ((void *)0)' failed! make: *** [test] Error 127 To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/739785/+subscriptions
[Qemu-devel] [Bug 739785] Re: qemu-i386 user mode can't fork (bash: fork: Invalid argument)
Requirements to get Wine 1.5.11 and Qemu to work with Ubuntu 12.10 on ARM 1. VMSPLIT-3G and BINFMT_MISC must be compiled into the kernel.. It makes my kernel crash when I access the lan\wifi with traffic.. 2. Use Qemu-0.14.1 with the NPTL Patch ./configure --enable-sdl --target-list=i386-linux-user --prefix=/usr --extra-cflags=-marm to compile.. I compile it in Ubuntu for Arm. 3. Compile Wine 1.5.11 on Ubuntu 12.10 32bit X86. 4. Create /usr/gnemul/qemu-i386/lib /usr/gnemul/qemu-i386/usr/local/lib, /usr/gnemul/qemu-i386/usr/local/lib/wine /usr/gnemul/qemu-i386/usr/lib /usr/gnemul/qemu-i386/usr/lib/i386-linux-gnu and copy the corresponding files from X86. Qemu\Wine will ask for them. Bring over your ~/.wine directory and put it in your home folder. 5. sudo apt-get build-dep wine (do this on arm) 6 Binfmt time. Here is my script to get wine to run echo ':i386:M::\x7fELF\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\x03:\xff\xff\xff\xff\xff\xfe\xfe\xff\xff\xff\xff\xff\xff\xff\xff\xff\xfb\xff\xff:/usr/bin/qemu-i386:' >/proc/sys/fs/binfmt_misc/register echo ':DOSWin:M::MZ::/usr/local/bin/wine:' >/proc/sys/fs/binfmt_misc/register 7. If you do ALL that... Then winecfg will run. About to start testing programs. Pinball runs VERY FAST! VERY PLAYABLE NOW!!! :D Justin Shafer OnsiteDentalSystems.com -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/739785 Title: qemu-i386 user mode can't fork (bash: fork: Invalid argument) Status in QEMU: New Status in “qemu” package in Debian: Confirmed Bug description: Good time of day everybody, I have been trying to make usermode qemu on ARM with plugapps (archlinux) with archlinux i386 chroot to work. 1. I installed arch linux in a virtuabox and created a chroot for it with mkarchroot. Transferred it to my pogo plug into /i386/ 2. I comiled qemu-i386 static and put it into /i386/usr/bin/ ./configure --static --disable-blobs --disable-system --target-list=i386-linux-user make 3. I also compiled linux kernel 2.6.38 with CONFIG_BINFMT_MISC=y and installed it. uname -a Linux Plugbox 2.6.38 #4 PREEMPT Fri Mar 18 22:19:10 CDT 2011 armv5tel Feroceon 88FR131 rev 1 (v5l) Marvell SheevaPlug Reference Board GNU/Linux 4. Added the following options into /etc/rc.local /sbin/modprobe binfmt_misc /bin/mount binfmt_misc -t binfmt_misc /proc/sys/fs/binfmt_misc echo ':qemu-i386:M::\x7fELF\x01\x01\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\x03\x00:\xff\xff\xff\xff\xff\xfe\xfe\xff\xff\xff\xff\xff\xff\xff\xff\xff\xfb\xff\xff\xff:/usr/bin/qemu-i386:' >/proc/sys/fs/binfmt_misc/register 5. Also copied ld-linux.so.3 (actually ld-2.13.so because ld- linux.so.3 is a link to that file) from /lib/ to /i386/lib/ 6.Now i chroot into /i386 and I get this: [root@Plugbox i386]# chroot . [II aI hnve ao n@P /]# pacman -Suy bash: fork: Invalid argument 7.I also downloaded linux-user-test-0.3 from qemu website and ran the test: [root@Plugbox linux-user-test-0.3]# make ./qemu-linux-user.sh [qemu-i386] ../qemu-0.14.0/i386-linux-user/qemu-i386 -L ./gnemul/qemu-i386 i386/ls -l dummyfile BUG IN DYNAMIC LINKER ld.so: dl-version.c: 210: _dl_check_map_versions: Assertion `needed != ((void *)0)' failed! make: *** [test] Error 127 To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/739785/+subscriptions
[Qemu-devel] buildbot failure in qemu on block_mingw32
The Buildbot has detected a new failure on builder block_mingw32 while building qemu. Full details are available at: http://buildbot.b1-systems.de/qemu/builders/block_mingw32/builds/390 Buildbot URL: http://buildbot.b1-systems.de/qemu/ Buildslave for this Build: kraxel_rhel61 Build Reason: The Nightly scheduler named 'nightly_block' triggered this build Build Source Stamp: [branch block] HEAD Blamelist: BUILD FAILED: failed compile sincerely, -The Buildbot
[Qemu-devel] [ANNOUNCE] We are now in hard freeze
At this point, we only are taking bug fixes until the final release of 1.3 on December 3rd. Submaintainers may continue to accept patches into their trees for new features at their discretion (although those pull requests will need to wait until 1.4 opens up). http://wiki.qemu.org/Planning/1.3 Regards, Anthony Liguori
[Qemu-devel] [ANNOUNCE] QEMU 1.3.0-rc0 is out!
Hi, On behalf of the QEMU Team, I'd like to announce the availability of the first release candidate for the QEMU 1.3 release. This release is meant for testing purposes and should not be used in a production environment. http://wiki.qemu.org/download/qemu-1.3.0-rc0.tar.bz2 You can help improve the quality of the QEMU 1.3 release by testing this release and reporting bugs on Launchpad: https://bugs.launchpad.net/qemu/ The release plan for the 1.3 release is available at: http://wiki.qemu.org/Planning/1.3 And a detailed change log is available at: http://wiki.qemu.org/ChangeLog/Next Regards, Anthony Liguori
Re: [Qemu-devel] [Qemu-ppc] [PATCH 08/12] target-ppc: Convert ppcemb_tlb_t to use fixed 64-bit RPN
On Mon, Nov 19, 2012 at 05:26:45PM +0100, Alexander Graf wrote: > > On 13.11.2012, at 03:46, David Gibson wrote: > > > Currently the ppcemb_tlb_t struct, used on a number of embedded ppc models > > to represent a TLB entry contains a hwaddr. That works reasonably for now, > > but is troublesome for saving the state, which we'll want to do in future. > > hwaddr is a large enough type to contain a physical address for any > > supported machine - and can thus, in theory at least, vary depending on > > what machines are enabled other than the one we're actually using right > > now (though in fact it doesn't for ppc). This makes it unsuitable for > > describing in vmstate. > > > > This patch therefore changes ppcemb_tlb_t to use a fixed 64-bit integer > > which we know is sufficient for all the machines which use this structure. > > hwaddr is always defined to 64bit by now. I know, but there aren't state save helpers for hwaddr, and there are objections to creating them. -- 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
Re: [Qemu-devel] [Qemu-ppc] [PATCH 12/12] pseries: Generate unique LIOBNs for PCI host bridges
On Mon, Nov 19, 2012 at 05:34:12PM +0100, Alexander Graf wrote: > > On 13.11.2012, at 03:47, David Gibson wrote: > > > From: Alexey Kardashevskiy > > > > In future (with VFIO) we will have multiple PCI host bridges on > > pseries. Each one needs a unique LIOBN (IOMMU id). At the moment we > > derive these from the pci domain number, but the whole notion of > > domain numbers on the qemu side is bogus and in any case they're not > > actually uniquely allocated at this point. > > > > This patch, therefore uses a simple sequence counter to generate > > unique LIOBNs for PCI host bridges. > > > > Signed-off-by: Alexey Kardashevskiy > > Signed-off-by: David Gibson > > I don't really like the idea of having a global variable just > because our domain ID generation seems to not work as > expected. Michael, any comments here? Well, the patch I sent which changed domain id generation was ignored. In any case, as I said, the whole concept of domain numbers makes no sense on the qemu side, so I don't think increasing reliance on them by using them here is a good idea. It would be conceptually nicer to derive the liobn from the buid, but that would rely on the buid's being unique in the low 32-bits, which is true in practice, but seems risky to rely on. -- 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
Re: [Qemu-devel] [Qemu-ppc] [PATCH 03/12] pseries: Move XICS initialization before cpu initialization
On Tue, Nov 20, 2012 at 06:54:20AM +1100, Benjamin Herrenschmidt wrote:
> On Mon, 2012-11-19 at 17:22 +0100, Alexander Graf wrote:
> > > Currently, the pseries machine initializes the cpus, then the XICS
> > > interrupt controller. However, to support the upcoming in-kernel XICS
> > > implementation we will need to initialize the irq controller before the
> > > vcpus. This patch makes the necesssary rearrangement. This means the
> > > xics init code can no longer auto-detect the number of cpus ("interrupt
> > > servers" in XICS terminology) and so we must pass that in explicitly from
> > > the platform code.
> >
> > Does this still hold true with the new in-kernel interrupt controller
> > workflow?
>
> We need to look into this. The in-kernel ICPs will still certainly be
> created early along with the VCPUs, however we might be able to delay
> the creation of the qemu emulation when not using the former.
I'd really prefer not to have two different initialization orders in
qemu though. I think we'll want the ICP initialization first for CPU
hotplug one day anyway.
--
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
Re: [Qemu-devel] BlockDriverState member in_use (was: KVM Forum 2012 Block BoF minutes)
On Mon, Nov 19, 2012 at 06:03:55PM +0100, Markus Armbruster wrote:
> Marcelo Tosatti writes:
>
> > On Thu, Nov 15, 2012 at 02:31:53PM +0100, Markus Armbruster wrote:
> [...]
> >> BlockDriverState member in_use and DriveInfo member refcount are a mess:
> >>
> >> - in_use is used to avoid running certain things concurrently, but the
> >> rules are unclear, except they're clearly incomplete; possible rules
> >> could be about need for consistent views of image contents
> >
> > block: enable in_use flag
> >
> > Set block device in use during block migration, disallow drive_del and
> > bdrv_truncate for in use devices.
> >
> > 1) drive_del is not allowed because memory object used by block
> > streaming/migration can disappear.
> > 2) bdrv_truncate changes device size, which the block migration code was
> > unable to handle at the time.
> >
> > These are the rules (accordingly to the changeset).
>
> Yup. Your commits db593f25^..8591675f.
>
> > IIRC new rules have
> > been added (new uses for bdrv_in_use).
>
> Stefan's commit 2d3735d3.
>
> Let's have a look at the users of in_use (from notes I took back in
> August, possibly stale now):
>
> * bdrv_commit()
>
> Since commit 2d3735d3:
>
> block: check bdrv_in_use() before blockdev operations
>
> Long-running block operations like block migration and image streaming
> must have continual access to their block device. It is not safe to
> perform operations like hotplug, eject, change, resize, commit, or
> external snapshot while a long-running operation is in progress.
>
> This patch adds the missing bdrv_in_use() checks so that block migration
> and image streaming never have the rug pulled out from underneath
> them.
>
> Users are monitor command commit and terminal escape 's', directly and
> via bdrv_commit_all()
>
> What exactly would go wrong if someone commited during block migration
> / image streaming?
if (ro) {
if (bdrv_reopen(bs->backing_hd, open_flags | BDRV_O_RDWR, NULL)) {
return -EACCES;
}
}
> Strange: also tests whether backing_hd is in_use. The other users
> don't, and I can't see how the test could ever succeed.
>
> Cock-up: bdrv_commit_all() stops on first error.
>
> * bdrv_truncate()
>
> Since commit 8591675f:
>
> block: enable in_use flag
>
> Set block device in use during block migration, disallow drive_del and
> bdrv_truncate for in use devices.
>
> Users are monitor command block_resize and a bunch of block drivers.
>
> I don't think block drivers are affected by in_use. They resize their
> internal BDSs such as bs->file, and I can't see how in_use could be
> set there.
>
> "No resize while block migration is running" is (was?) an artificial
> restriction; it should migrate the resize just like any other change.
> Image streaming, too, I guess.
If its necessary to allow concurrent operation, then yes.
> * block_job_create()
>
> Since commit eeec61f2:
>
> block: add BlockJob interface for long-running operations
>
> Use is monitor command block-stream.
>
> Why does the block job infrastructure set in_use? I suspect just
> because its only user "image streaming" needs it, but future users
> might not. If that's correct, it's set in the wrong place.
Because block stream supposedly does not handle a BDS changing size (and
it was not necessary to handle this case at the time). Changing it
requires auditing the code.
> * qmp_transaction()
>
> Since commit 2d3735d3 (see above). Function was called
> qmp_blockdev_snapshot_sync() then.
>
> User is monitor command snapshot_blkdev.
>
> What exactly would go wrong if someone snaphotted during block
> migration / image streaming?
>
> * eject_device()
>
> Since commit 2d3735d3 (see above).
>
> Users are monitor command eject, change.
>
> What would go wrong is obvious, for a change: we can't just kill the
> BDS while a long-running operation is using it.
>
> Could we just disassociate the BDS from the drive without killing it?
> So that when the job completes, the BDS's reference count drops to
> zero, and the BDS gets destroyed.
>
> * drive_del
>
> Since commit 8591675f (see above).
>
> In theory, in_use is unnecessary here: reference counting should delay
> the delete just fine. In practice, do_drive_del() *ignores* the
> reference count. Even if that was fixed, the count only delays
> drive_uninit(), not bdrv_drain_all()..bdrv_close(). Another fine
> mess.
>
> Note how we treat the device model's reference to the drive: if it
> exists, we make the drive anonymous instead of destroying it. It gets
> destroyed only when the device model drops the reference. Similar to
> reference counting.
>
> Tests of in_use are spread over block.c and monitor commands. Smells
> bad. If it was only about excluding m
Re: [Qemu-devel] [PATCH] tci: Fix type of tci_read_label
Am 19.11.2012 21:43, schrieb Richard Henderson:
Fixes the pointer truncation that was occurring for branches.
Cc: Stefan Weil
Cc: Blue Swirl
Signed-off-by: Richard Henderson
---
tci.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tci.c b/tci.c
index 9c87c8e..54cf1d9 100644
--- a/tci.c
+++ b/tci.c
@@ -338,9 +338,9 @@ static uint64_t tci_read_ri64(uint8_t **tb_ptr)
}
#endif
-static target_ulong tci_read_label(uint8_t **tb_ptr)
+static tcg_target_ulong tci_read_label(uint8_t **tb_ptr)
{
-target_ulong label = tci_read_i(tb_ptr);
+tcg_target_ulong label = tci_read_i(tb_ptr);
assert(label != 0);
return label;
}
Thanks for fixing this.
Reviewed-by: Stefan Weil
Tested-by: Stefan Weil
Re: [Qemu-devel] no monitor after disable vnc
Hi --enable-sdl can come up with a monitor screen, thanks Thanksfrom Peter > Date: Mon, 19 Nov 2012 14:38:45 +0100 > From: pbonz...@redhat.com > To: mcheun...@hotmail.com > CC: qemu-devel@nongnu.org > Subject: Re: no monitor after disable vnc > > Il 19/11/2012 14:25, Peter Cheung ha scritto: > > Dear All > > When i compile qemu in ubuntu 12.10 with "./configure > > --target-list=i386-softmmu --prefix=/root/qemu --enable-debug > > --disable-vnc --disable-werror" , after i start qemu, no screen output, > > how to enable it? > > Please attach your config.log. > > Paolo
[Qemu-devel] [PATCH] rbd block driver fix race between aio completition and aio cancel
From: Stefan Priebe
This one fixes a race qemu also had in iscsi block driver between
cancellation and io completition.
qemu_rbd_aio_cancel was not synchronously waiting for the end of
the command.
It also removes the useless cancelled flag and introduces instead
a status flag with EINPROGRESS like iscsi block driver.
Signed-off-by: Stefan Priebe
---
block/rbd.c | 19 ---
1 file changed, 12 insertions(+), 7 deletions(-)
diff --git a/block/rbd.c b/block/rbd.c
index 5a0f79f..7b3bcbb 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -76,7 +76,7 @@ typedef struct RBDAIOCB {
int64_t sector_num;
int error;
struct BDRVRBDState *s;
-int cancelled;
+int status;
} RBDAIOCB;
typedef struct RADOSCB {
@@ -376,9 +376,7 @@ static void qemu_rbd_complete_aio(RADOSCB *rcb)
RBDAIOCB *acb = rcb->acb;
int64_t r;
-if (acb->cancelled) {
-qemu_vfree(acb->bounce);
-qemu_aio_release(acb);
+if (acb->bh) {
goto done;
}
@@ -406,9 +404,12 @@ static void qemu_rbd_complete_aio(RADOSCB *rcb)
acb->ret = r;
}
}
+acb->status = acb->ret;
+
/* Note that acb->bh can be NULL in case where the aio was cancelled */
acb->bh = qemu_bh_new(rbd_aio_bh_cb, acb);
qemu_bh_schedule(acb->bh);
+
done:
g_free(rcb);
}
@@ -573,7 +574,10 @@ static void qemu_rbd_close(BlockDriverState *bs)
static void qemu_rbd_aio_cancel(BlockDriverAIOCB *blockacb)
{
RBDAIOCB *acb = (RBDAIOCB *) blockacb;
-acb->cancelled = 1;
+
+while (acb->status == -EINPROGRESS) {
+qemu_aio_wait();
+}
}
static AIOPool rbd_aio_pool = {
@@ -642,10 +646,11 @@ static void rbd_aio_bh_cb(void *opaque)
qemu_iovec_from_buf(acb->qiov, 0, acb->bounce, acb->qiov->size);
}
qemu_vfree(acb->bounce);
-acb->common.cb(acb->common.opaque, (acb->ret > 0 ? 0 : acb->ret));
qemu_bh_delete(acb->bh);
acb->bh = NULL;
+acb->common.cb(acb->common.opaque, (acb->ret > 0 ? 0 : acb->ret));
+
qemu_aio_release(acb);
}
@@ -689,8 +694,8 @@ static BlockDriverAIOCB *rbd_start_aio(BlockDriverState *bs,
acb->ret = 0;
acb->error = 0;
acb->s = s;
-acb->cancelled = 0;
acb->bh = NULL;
+acb->status = -EINPROGRESS;
if (cmd == RBD_AIO_WRITE) {
qemu_iovec_to_buf(acb->qiov, 0, acb->bounce, qiov->size);
--
1.7.10.4
[Qemu-devel] (no subject)
>From Stefan Priebe # This line is ignored. From: Stefan Priebe Cc: pve-de...@pve.proxmox.com Cc: pbonz...@redhat.com Cc: ceph-de...@vger.kernel.org Subject: QEMU/PATCH: rbd block driver: fix race between completition and cancel In-Reply-To: ve-de...@pve.proxmox.com pbonz...@redhat.com ceph-de...@vger.kernel.org
[Qemu-devel] [PATCH] tci: Fix type of tci_read_label
Fixes the pointer truncation that was occurring for branches.
Cc: Stefan Weil
Cc: Blue Swirl
Signed-off-by: Richard Henderson
---
tci.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tci.c b/tci.c
index 9c87c8e..54cf1d9 100644
--- a/tci.c
+++ b/tci.c
@@ -338,9 +338,9 @@ static uint64_t tci_read_ri64(uint8_t **tb_ptr)
}
#endif
-static target_ulong tci_read_label(uint8_t **tb_ptr)
+static tcg_target_ulong tci_read_label(uint8_t **tb_ptr)
{
-target_ulong label = tci_read_i(tb_ptr);
+tcg_target_ulong label = tci_read_i(tb_ptr);
assert(label != 0);
return label;
}
--
1.7.11.7
Re: [Qemu-devel] [GitHub] [Qemu-commits] [qemu/qemu] ecdffb: tcg/ppc: Remove unused s_bits variable
On Mon, 19 Nov 2012, Anthony Liguori wrote: > > "Public domain" is not a valid copyright header/license grant. Sez who? > > I'd suggest using something like CC0 which is a formal statement of > no copyright claims. > > http://www.ohloh.net/licenses/cc0-1-universal > > Please send these sorts of changes to the mailing list first too. > > Regards, > > Anthony Liguori > > -- mailto:av1...@comtv.ru
[Qemu-devel] [GitHub] [Qemu-commits] [qemu/qemu] ecdffb: tcg/ppc: Remove unused s_bits variable
"Public domain" is not a valid copyright header/license grant. I'd suggest using something like CC0 which is a formal statement of no copyright claims. http://www.ohloh.net/licenses/cc0-1-universal Please send these sorts of changes to the mailing list first too. Regards, Anthony Liguori --- Begin Message --- Branch: refs/heads/master Home: https://github.com/qemu/qemu Commit: ecdffbccd783cd79eb3ce206375270de54046ea0 https://github.com/qemu/qemu/commit/ecdffbccd783cd79eb3ce206375270de54046ea0 Author: malc Date: 2012-11-19 (Mon, 19 Nov 2012) Changed paths: M tcg/ppc/tcg-target.c Log Message: --- tcg/ppc: Remove unused s_bits variable Thanks to Alexander Graf for heads up. Signed-off-by: malc Commit: 456a84d156a7c42f18b1da176dd6219e2dffd043 https://github.com/qemu/qemu/commit/456a84d156a7c42f18b1da176dd6219e2dffd043 Author: malc Date: 2012-11-19 (Mon, 19 Nov 2012) Changed paths: M audio/wavcapture.c Log Message: --- audio/wavcapture: Clarify licensing Signed-off-by: malc Commit: 72bc6f1bf710e205f175af9b1fc8bbd83e8da71f https://github.com/qemu/qemu/commit/72bc6f1bf710e205f175af9b1fc8bbd83e8da71f Author: malc Date: 2012-11-19 (Mon, 19 Nov 2012) Changed paths: M audio/audio_pt_int.c Log Message: --- audio/audio_pt_int: Clarify licensing Signed-off-by: malc Compare: https://github.com/qemu/qemu/compare/a36e9561283e...72bc6f1bf710 --- End Message ---
Re: [Qemu-devel] [PATCH] LICENSE: clarify licensing
On Mon, 19 Nov 2012, Anthony Liguori wrote: > malc writes: > > > On Mon, 19 Nov 2012, Peter Maydell wrote: > > > >> On 19 November 2012 18:21, malc wrote: > >> > On Mon, 19 Nov 2012, Anthony Liguori wrote: > >> >> +5) Files without explicit licenses fall under the GPL v2. > >> > > >> > I have issue with this, files without licenses are just that files > >> > without licenses. > >> > >> If we believe this (and it seems a logical thing to believe) > >> then QEMU's not distributable until we rewrite or remove or track > >> down all authors for all the files without licenses... > > > > Yes. > > That's ridiculous. > > There has always been a LICENSE file with a catch-all clause going back > to at least 2005. > > If a file doesn't have an explicit LICENSE, it falls under the catch all > clause. > Wishful thinking. -- mailto:av1...@comtv.ru
Re: [Qemu-devel] [PATCH 03/12] pseries: Move XICS initialization before cpu initialization
On Mon, 2012-11-19 at 17:22 +0100, Alexander Graf wrote:
> > Currently, the pseries machine initializes the cpus, then the XICS
> > interrupt controller. However, to support the upcoming in-kernel XICS
> > implementation we will need to initialize the irq controller before the
> > vcpus. This patch makes the necesssary rearrangement. This means the
> > xics init code can no longer auto-detect the number of cpus ("interrupt
> > servers" in XICS terminology) and so we must pass that in explicitly from
> > the platform code.
>
> Does this still hold true with the new in-kernel interrupt controller
> workflow?
We need to look into this. The in-kernel ICPs will still certainly be
created early along with the VCPUs, however we might be able to delay
the creation of the qemu emulation when not using the former.
Cheers,
Ben.
Re: [Qemu-devel] [PATCH] LICENSE: clarify licensing
malc writes: > On Mon, 19 Nov 2012, Peter Maydell wrote: > >> On 19 November 2012 18:21, malc wrote: >> > On Mon, 19 Nov 2012, Anthony Liguori wrote: >> >> +5) Files without explicit licenses fall under the GPL v2. >> > >> > I have issue with this, files without licenses are just that files >> > without licenses. >> >> If we believe this (and it seems a logical thing to believe) >> then QEMU's not distributable until we rewrite or remove or track >> down all authors for all the files without licenses... > > Yes. That's ridiculous. There has always been a LICENSE file with a catch-all clause going back to at least 2005. If a file doesn't have an explicit LICENSE, it falls under the catch all clause. Regards, Anthony Liguori > > -- > mailto:av1...@comtv.ru
Re: [Qemu-devel] [PATCH] LICENSE: clarify licensing
On Mon, 19 Nov 2012, Stefan Weil wrote: > Am 19.11.2012 19:34, schrieb malc: > > On Mon, 19 Nov 2012, Peter Maydell wrote: > > > > > On 19 November 2012 18:21, malc wrote: > > > > On Mon, 19 Nov 2012, Anthony Liguori wrote: > > > > > +5) Files without explicit licenses fall under the GPL v2. > > > > > > > > I have issue with this, files without licenses are just that files > > > > without licenses. > > > > > > If we believe this (and it seems a logical thing to believe) > > > then QEMU's not distributable until we rewrite or remove or track > > > down all authors for all the files without licenses... > > > > Yes. > > That can only be true if those files are older than LICENSE, > or at least older than the commit which added > > "QEMU as a whole is released under the GNU General Public License". (2007) > > Any file or contribution which was added later (with or without a license > clause) > cannot invalidate this general license, so QEMU remains distributable. > I think you are mistaken, this just clarifies the redistributability of the QEMU as a whole, given that it uses some code which has strong claims (GPL) it as a whole must adhere to these. If it contains code that has no license it just can not use that code, nothing GPL can do about it. > Nevertheless fixing files without explicit license is desirable, > of course. > > Stefan > -- mailto:av1...@comtv.ru
Re: [Qemu-devel] [PATCH] LICENSE: clarify licensing
Am 19.11.2012 19:34, schrieb malc: On Mon, 19 Nov 2012, Peter Maydell wrote: On 19 November 2012 18:21, malc wrote: On Mon, 19 Nov 2012, Anthony Liguori wrote: +5) Files without explicit licenses fall under the GPL v2. I have issue with this, files without licenses are just that files without licenses. If we believe this (and it seems a logical thing to believe) then QEMU's not distributable until we rewrite or remove or track down all authors for all the files without licenses... Yes. That can only be true if those files are older than LICENSE, or at least older than the commit which added "QEMU as a whole is released under the GNU General Public License". (2007) Any file or contribution which was added later (with or without a license clause) cannot invalidate this general license, so QEMU remains distributable. Nevertheless fixing files without explicit license is desirable, of course. Stefan
Re: [Qemu-devel] [PATCHv3 0/2] Preparing safe sigprocmask wrapper on qemu-user
ping
On Sat, Oct 20, 2012 at 4:15 PM, Alex Barcelo wrote:
> qemu-user needs SIGSEGV (at least) for some internal use. If the guest
> application masks it and does unsafe sigprocmask, then the application
> crashes. Problems happen in applications with self-modifying code (who
> also change the signal mask). Other guest applications may have related
> problems if they use the SIGSEGV.
>
> A way to be more safe is adding a wrapper for all sigprocmask calls from
> the guest. The wrapper proposed here is quite simple, but the code can
> be improved, here I try to ensure that the wrapper is set up properly.
>
> Changes in v3:
> - Wrapping also sigreturn's sigprocmask calls (on signal.c)
>
> Here, a test case where qemu-user goes wrong:
>
>
> #include
> #include
> #include
> #include
> #include
> #include
>
> unsigned char *testfun;
>
> int main ( void )
> {
> unsigned int ra;
> testfun=memalign(getpagesize(),1024);
> // We block the SIGSEGV signal, used by qemu-user
> sigset_t set;
> sigemptyset(&set);
> sigaddset(&set, 11);
> sigprocmask(SIG_BLOCK, &set, NULL);
> mprotect(testfun, 1024, PROT_READ|PROT_EXEC|PROT_WRITE);
>
> //400687: b8 0d 00 00 00 mov$0xd,%eax
> //40068d: c3 retq
> testfun[ 0]=0xb8;
> testfun[ 1]=0x0d;
> testfun[ 2]=0x00;
> testfun[ 3]=0x00;
> testfun[ 4]=0x00;
> testfun[ 5]=0xc3;
> printf ( "0x%02X\n",
> ((unsigned int (*)())testfun)() );
>
> //400687: b8 20 00 00 00 mov$0x20,%eax
> //40068d: c3 retq
> // This self-modifying code will break because of the sigsegv signal
> block
> testfun[ 1]=0x20;
> printf ( "0x%02X\n",
> ((unsigned int (*)())testfun)() );
> }
>
>
> On an i386 native host:
> 0x0D
> 0x20
>
> On a non-patched qemu-i386:
> 0x0D
> Segmentation fault
>
> Alex Barcelo (2):
> signal: added a wrapper for sigprocmask function
> signal: sigsegv protection on do_sigprocmask
>
> linux-user/qemu.h|1 +
> linux-user/signal.c | 27 +++
> linux-user/syscall.c | 14 +++---
> 3 files changed, 35 insertions(+), 7 deletions(-)
>
> --
> 1.7.5.4
>
>
Re: [Qemu-devel] [PATCH] LICENSE: clarify licensing
On Mon, 19 Nov 2012, Peter Maydell wrote: > On 19 November 2012 18:21, malc wrote: > > On Mon, 19 Nov 2012, Anthony Liguori wrote: > >> +5) Files without explicit licenses fall under the GPL v2. > > > > I have issue with this, files without licenses are just that files > > without licenses. > > If we believe this (and it seems a logical thing to believe) > then QEMU's not distributable until we rewrite or remove or track > down all authors for all the files without licenses... Yes. -- mailto:av1...@comtv.ru
Re: [Qemu-devel] [PATCH] LICENSE: clarify licensing
On 19 November 2012 18:21, malc wrote: > On Mon, 19 Nov 2012, Anthony Liguori wrote: >> +5) Files without explicit licenses fall under the GPL v2. > > I have issue with this, files without licenses are just that files > without licenses. If we believe this (and it seems a logical thing to believe) then QEMU's not distributable until we rewrite or remove or track down all authors for all the files without licenses... -- PMM
Re: [Qemu-devel] [PATCH] LICENSE: clarify licensing
On Mon, 19 Nov 2012, Anthony Liguori wrote: > There's a couple changes here. Please note that no one should consider this > as > licensing guidance from IBM. I'm merely updating this based on what I would > consider community consensus. > > The first change clarifies that the overall license is GPLv2. We have never > had GPLv1 code. The phrase "GNU General Public License" clearly refers to the > GPLv2. We cannot say the overall license is "GPLv2 or later" because we have > a number of files that are GPLv2 only. > > The second change clarifies that any file that doesn't have an explicit > license > is covered under the GPLv2. This agrees with the first statement in LICENSE > but > since it's so often asked, I thought we should make it explicit. > > I also added an additional statement that this file only expresses the intent > of the QEMU community and should only be considered informative. > > Please explicitly Nack or Ack this change. > > Signed-off-by: Anthony Liguori > --- > LICENSE | 8 ++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/LICENSE b/LICENSE > index acae9a3..f4bee4c 100644 > --- a/LICENSE > +++ b/LICENSE > @@ -1,6 +1,6 @@ > The following points clarify the QEMU license: > > -1) QEMU as a whole is released under the GNU General Public License > +1) QEMU as a whole is released under the GNU General Public License version 2 > > 2) Parts of QEMU have specific licenses which are compatible with the > GNU General Public License. Hence each source file contains its own > @@ -13,4 +13,8 @@ Many hardware device emulation sources are released under > the BSD license. > > 4) QEMU is a trademark of Fabrice Bellard. > > -Fabrice Bellard. > +5) Files without explicit licenses fall under the GPL v2. I have issue with this, files without licenses are just that files without licenses. > + > +6) This file represents the consensus view of the QEMU community. It does > not > + alter any licenses used in the code base and should be viewed as > informative > + only. > -- mailto:av1...@comtv.ru
Re: [Qemu-devel] [PATCH 0/1] s390 files gplv2 or later
On 19.11.2012, at 19:17, Paolo Bonzini wrote: > Il 19/11/2012 18:31, Alexander Graf ha scritto: >> Given that GPLv2 only code is compatible with LGPLv2+, can't you just >> declare all IBM s390x code to be GPLv2 only? > > What Christian did matches what we have done for all files, so I'd > prefer that. Ah, ok :). I'll apply it then with fixed year. Alex
Re: [Qemu-devel] [PATCH 0/1] s390 files gplv2 or later
Il 19/11/2012 18:31, Alexander Graf ha scritto: > Given that GPLv2 only code is compatible with LGPLv2+, can't you just > declare all IBM s390x code to be GPLv2 only? What Christian did matches what we have done for all files, so I'd prefer that. Paolo > Then the rest should > solve itself automatically, no? But then again, I'm not a lawyer :).
Re: [Qemu-devel] [PATCH] LICENSE: clarify licensing
Il 19/11/2012 17:40, Anthony Liguori ha scritto: > There's a couple changes here. Please note that no one should consider this > as > licensing guidance from IBM. I'm merely updating this based on what I would > consider community consensus. > > The first change clarifies that the overall license is GPLv2. We have never > had GPLv1 code. The phrase "GNU General Public License" clearly refers to the > GPLv2. We cannot say the overall license is "GPLv2 or later" because we have > a number of files that are GPLv2 only. > > The second change clarifies that any file that doesn't have an explicit > license > is covered under the GPLv2. This agrees with the first statement in LICENSE > but > since it's so often asked, I thought we should make it explicit. No, files that don't have an explicit license are under "GPLv2 or later". > I also added an additional statement that this file only expresses the intent > of the QEMU community and should only be considered informative. > > Please explicitly Nack or Ack this change. > > Signed-off-by: Anthony Liguori > --- > LICENSE | 8 ++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/LICENSE b/LICENSE > index acae9a3..f4bee4c 100644 > --- a/LICENSE > +++ b/LICENSE > @@ -1,6 +1,6 @@ > The following points clarify the QEMU license: > > -1) QEMU as a whole is released under the GNU General Public License > +1) QEMU as a whole is released under the GNU General Public License version 2 > > 2) Parts of QEMU have specific licenses which are compatible with the > GNU General Public License. Hence each source file contains its own > @@ -13,4 +13,8 @@ Many hardware device emulation sources are released under > the BSD license. > > 4) QEMU is a trademark of Fabrice Bellard. > > -Fabrice Bellard. > +5) Files without explicit licenses fall under the GPL v2. > + > +6) This file represents the consensus view of the QEMU community. It does > not > + alter any licenses used in the code base ... for individual files... > + and should be viewed as informative > + only. > Paolo
Re: [Qemu-devel] [PATCH 0/2] i2c: Add AT24Cxx EEPROM model
Am 19.11.2012 15:24, schrieb Jan Kiszka: See patches for details. Jan Kiszka (2): i2c: Introduce device address mask Add AT24Cxx I2C EEPROM device model Is AT24Cxx compatible to Microchip 24C02SC or SGS Thomson ST24C02 (which are emulated in hw/mips_malta.c)? Could we share code? Stefan
Re: [Qemu-devel] [Qemu-trivial] [PATCH] block: Fix regression for MinGW (assertion caused by short string)
Am 19.11.2012 12:31, schrieb Stefan Hajnoczi:
On Sun, Nov 18, 2012 at 09:26:15AM +0100, Stefan Weil wrote:
The local string tmp_filename is passed to function get_tmp_filename
which expects a string with minimum size MAX_PATH for w32 hosts.
MAX_PATH is 260 and PATH_MAX is 259, so tmp_filename was too short.
Commit eba25057b9a5e19d10ace2bc7716667a31297169 introduced this
regression.
Signed-off-by: Stefan Weil
---
block.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block.c b/block.c
index 49dd6bb..8739635 100644
--- a/block.c
+++ b/block.c
@@ -787,7 +787,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename,
int flags,
BlockDriver *drv)
{
int ret;
-char tmp_filename[PATH_MAX];
+char tmp_filename[PATH_MAX + 1];
This is not maintainable - the patch is making an assumption about the relative
values of MAX_PATH and PATH_MAX. There is no comment explaining this so it's
likely to be changed and break in the future again.
The relation between MAX_PATH and PATH_MAX seems to be well definedand
is valid for w32 and w64, so there is no need to make any assumption:
buffers must allocate MAX_PATH elements for up to PATH_MAX
character entries plus a terminating 0.
I considered writing a comment, but decided not to do so because
caller and called function are in the same file and most people
are not very interested in Windows peculiarities.
The code in block/vvfat.c which uses a length of 1024without
explanation is worse.
A clean solution is to change get_tmp_filename() so the caller doesn't need to
pass in a fixed size:
/*
* Create a uniquely-named empty temporary file.
* Return 0 upon success, otherwise a negative errno value.
*
* The filename must be freed with g_free() when it is no longer needed.
*/
int get_tmp_filename(char **filename)
For block/vvfat, that would even simplify the code.
The existing get_tmp_filename() code has another problem. Here is the Windows
get_tmp_filename() code:
char temp_dir[MAX_PATH];
/* GetTempFileName requires that its output buffer (4th param)
have length MAX_PATH or greater. */
assert(size >= MAX_PATH);
return (GetTempPath(MAX_PATH, temp_dir)
&& GetTempFileName(temp_dir, "qem", 0, filename)
? 0 : -GetLastError());
The assert has an off-by-one error because the documentation says:
"This buffer should be MAX_PATH characters to accommodate the path plus the
terminating null character."
http://msdn.microsoft.com/en-us/library/windows/desktop/aa364991(v=vs.85).aspx
No. The documentation can be read in two ways:
"This buffer should be (MAX_PATH characters) to accommodate (the path plus the
terminating null character)."
"This buffer should be (MAX_PATH characters to accommodate the path) plus (the
terminating null character)."
The first one matches the current code and also the example from MS:
http://msdn.microsoft.com/en-us/library/windows/desktop/aa363875%28v=vs.85%29.aspx
In a short experiment, I was able to create filenames with up to 228
characters
using GetTempFileName, so even 229 bytes instead of MAX_PATH (260) would
be sufficient.
Since the function returns -errno the assert could be turned into:
/* GetTempFileName requires that its output buffer (4th param)
have length MAX_PATH + 1 or greater. */
if (size < MAX_PATH + 1) {
return -ENOSPC;
}
Stefan
"if (size < MAX_PATH) {"
I'd still prefer the assertion because that is not a general purpose
library function but a QEMU internal function which must be called
with correct parameters. Here raising an assertion is better than
silently returning an error status. Of course we could do both.
Any patch which fixes this regression for MinGW is fine -
my patch was simply the smallest possible change to achieve this goal,
and I still think that it is sufficient.
If we want a better solution, we should also consider g_mkstemp
and related functions like g_file_open_tmp. We could also modify
bdrv_create to allow a NULL filename which is interpreted as a
temporary filename. IMHO that would be a good solution for the
future (= after 1.3). Feel free to add a TODO comment to the
code in my patch.
Regards,
Stefan
Re: [Qemu-devel] [RFC PATCH 1/3] virtio-bus : Introduce VirtioBus.
On 16 November 2012 15:35, wrote:
> From: KONRAD Frederic
You forgot to CC enough people...
> This patch create a new VirtioBus, which can be added to Virtio transports
> like
> virtio-pci, virtio-mmio,...
>
> One VirtIODevice can be connected to this device, like virtio-blk in the 3rd
> patch.
>
> The VirtioBus shares through a VirtioBusInfo structure :
>
> * two callbacks with the transport : init_cb and exit_cb, which must be
> called by the VirtIODevice, after the initialization and before the
> destruction, to put the right PCI IDs and/or stop the event fd.
>
> * a VirtIOBindings structure.
>
> Signed-off-by: KONRAD Frederic
> ---
> hw/Makefile.objs | 1 +
> hw/virtio-bus.c | 111
> +++
> hw/virtio-bus.h | 49
> 3 files changed, 161 insertions(+)
> create mode 100644 hw/virtio-bus.c
> create mode 100644 hw/virtio-bus.h
>
> diff --git a/hw/Makefile.objs b/hw/Makefile.objs
> index af4ab0c..1b25d77 100644
> --- a/hw/Makefile.objs
> +++ b/hw/Makefile.objs
> @@ -1,6 +1,7 @@
> common-obj-y = usb/ ide/
> common-obj-y += loader.o
> common-obj-$(CONFIG_VIRTIO) += virtio-console.o
> +common-obj-$(CONFIG_VIRTIO) += virtio-bus.o
> common-obj-$(CONFIG_VIRTIO_PCI) += virtio-pci.o
> common-obj-y += fw_cfg.o
> common-obj-$(CONFIG_PCI) += pci.o pci_bridge.o pci_bridge_dev.o
> diff --git a/hw/virtio-bus.c b/hw/virtio-bus.c
> new file mode 100644
> index 000..b2e7e9c
> --- /dev/null
> +++ b/hw/virtio-bus.c
> @@ -0,0 +1,111 @@
> +/*
> + * VirtioBus
> + *
> + * Copyright (C) 2012 : GreenSocs Ltd
> + * http://www.greensocs.com/ , email: i...@greensocs.com
> + *
> + * Developed by :
> + * Frederic Konrad
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2. See
> + * the COPYING file in the top-level directory.
Unless you copied this code from an existing v2-only file, preferred
license for new code is v2-or-any-later-version.
> + */
> +
> +#include "hw.h"
> +#include "qemu-error.h"
> +#include "qdev.h"
> +#include "virtio-bus.h"
> +#include "virtio.h"
> +
> +#define DEBUG_VIRTIO_BUS
> +
> +#ifdef DEBUG_VIRTIO_BUS
> +
> +#define DPRINTF(fmt, ...) \
> +do { printf("virtio_bus: " fmt , ## __VA_ARGS__); } while (0)
> +#else
> +#define DPRINTF(fmt, ...) do {} while (0)
> +#endif
> +
> +static char *virtio_bus_get_fw_dev_path(DeviceState *dev);
Is this function necessary? I think your implementation
is doing the same as the default.
> +static void virtio_bus_class_init(ObjectClass *klass, void *data)
> +{
> +BusClass *k = BUS_CLASS(klass);
> +k->get_fw_dev_path = virtio_bus_get_fw_dev_path;
> +}
> +
> +static const TypeInfo virtio_bus_info = {
> +.name = TYPE_VIRTIO_BUS,
> +.parent = TYPE_BUS,
> +.instance_size = sizeof(VirtioBus),
> +.class_init = virtio_bus_class_init,
> +};
> +
> +/* VirtioBus */
> +
> +static int next_virtio_bus;
> +
> +/* Create a virtio bus, and attach to transport. */
> +void virtio_bus_new(VirtioBus *bus, DeviceState *host,
> +const VirtioBusInfo *info)
> +{
> +/*
> + * Setting name to NULL return always "virtio.0"
> + * as bus name in info qtree.
> + */
> +char *bus_name = g_strdup_printf("%s.%d", BUS_NAME, next_virtio_bus);
> +qbus_create_inplace(&bus->qbus, TYPE_VIRTIO_BUS, host, bus_name);
> +bus->busnr = next_virtio_bus++;
This looks suspicious to me -- is keeping a count of the global
bus number really the right way to do this?
> +bus->info = info;
> +/* no hotplug for the moment ? */
> +bus->qbus.allow_hotplug = 0;
Correct -- we don't need to hotplug the virtio backend.
> +bus->bus_in_use = false;
> +DPRINTF("bus %s created\n", bus_name);
> +}
> +
> +/* Bind the VirtIODevice to the VirtioBus. */
> +void virtio_bus_bind_device(VirtioBus *bus)
> +{
> +assert(bus != NULL);
> +assert(bus->vdev != NULL);
> +virtio_bind_device(bus->vdev, &(bus->info->virtio_bindings),
> + bus->qbus.parent);
This looks wrong -- virtio_bind_device() is part of the
old-style transport API.
> +}
> +
> +/* This must be called to when the VirtIODevice init */
> +int virtio_bus_init_cb(VirtioBus *bus, VirtIODevice *vdev)
> +{
> +if (bus->bus_in_use == true) {
> +error_report("%s in use.\n", bus->qbus.name);
> +return -1;
> +}
> +assert(bus->info->init_cb != NULL);
> +/* keep the VirtIODevice in the VirtioBus */
> +bus->vdev = vdev;
This shouldn't be happening here, it should happen as
part of plugging the device into the bus.
> +bus->info->init_cb(bus->qbus.parent);
> +
> +bus->bus_in_use = true;
> +return 0;
> +}
> +
> +/* This must be called when the VirtIODevice exit */
> +void virtio_bus_exit_cb(VirtioBus *bus)
> +{
> +assert(bus->info->exit_cb != NULL);
> +bus->info->exit_cb(bus->qbus.parent);
> +bus->bus_in_use = false;
> +}
These shouldn't be necessary -- the VirtioDev
Re: [Qemu-devel] [PATCH 0/1] s390 files gplv2 or later
On 12.11.2012, at 12:44, Christian Borntraeger wrote: > IBMs s390 contributions were meant to to be gplv2 or later (since > we were contributing to qemu). Several of the s390 specific files > link to gpl code anyway so the ones in qemu repository will have > a hard time being strictly LGPL. > So lets clarify the licence statement for new contributions for > those files that we have touched multiple times or will likely > touch again. > > Since we contributed only to KVM, this patch does not touch files > that mostly deal with tcg. Please tell me if I should do that for > the full target-s390x tree. > > The alternative would be to change the licence directly (no > contributions after but a 1:1 replacement) for the files in the > qemu repository. But that would require an agreement of all > contributors due to "v2" vs "v2 or later". Opinions? Given that GPLv2 only code is compatible with LGPLv2+, can't you just declare all IBM s390x code to be GPLv2 only? Then the rest should solve itself automatically, no? But then again, I'm not a lawyer :). Alex
Re: [Qemu-devel] [PATCHv2] tap: reset vnet header size on open
On 13.11.2012, at 11:23, Michael S. Tsirkin wrote:
> For tap, we currently assume the vnet header size is 10
> (the default value) but that might not be the case
> if tap is persistent and has been used by qemu previously.
> To fix, set host header size in tap device on open.
>
> Signed-off-by: Michael S. Tsirkin
> ---
>
> This fixes the issue reported by Alexander Graf on the kvm forum.
> Alexander, could you confirm please?
Yup, works for me :).
Tested-by: Alexander Graf
Alex
> Thanks,
>
> net/tap.c | 7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/net/tap.c b/net/tap.c
> index df89caa..1abfd44 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -341,6 +341,13 @@ static TAPState *net_tap_fd_init(NetClientState *peer,
> s->using_vnet_hdr = 0;
> s->has_ufo = tap_probe_has_ufo(s->fd);
> tap_set_offload(&s->nc, 0, 0, 0, 0, 0);
> +/*
> + * Make sure host header length is set correctly in tap:
> + * it might have been modified by another instance of qemu.
> + */
> +if (tap_probe_vnet_hdr_len(s->fd, s->host_vnet_hdr_len)) {
> +tap_fd_set_vnet_hdr_len(s->fd, s->host_vnet_hdr_len);
> +}
> tap_read_poll(s, 1);
> s->vhost_net = NULL;
> return s;
> --
> MST
Re: [Qemu-devel] Ubuntu/Debian Installer + Virtio-SCSI -> Bad ram pointer
On Thu, Nov 8, 2012 at 4:26 PM, Peter Lieven wrote: > Has anyone any other idea what the cause could be or where to start? Hi Peter, I suggested posting the source tree you are building. Since you have applied patches yourself no one else is able to follow along with the gdb output or reproduce the issue accurately. Stefan
Re: [Qemu-devel] [PATCH V5 08/10] Create four opts list related functions
On Thu, Nov 01, 2012 at 05:12:28PM +0800, Dong Xu Wang wrote:
> +/* Create a new QemuOptsList and make its desc to the merge of first and
> second.
> + * It will allocate space for one new QemuOptsList plus enouth space for
> + * QemuOptDesc in first and second QemuOptsList. First argument's QemuOptDesc
> + * members take precedence over second's.
> + */
> +QemuOptsList *append_opts_list(QemuOptsList *first,
> + QemuOptsList *second)
> +{
> +size_t num_first_options, num_second_options;
> +QemuOptsList *dest = NULL;
> +int i = 0;
> +int index = 0;
> +
> +num_first_options = count_opts_list(first);
> +num_second_options = count_opts_list(second);
> +if (num_first_options + num_second_options == 0) {
> +return NULL;
> +}
> +
> +dest = g_malloc0(sizeof(QemuOptsList)
> ++ (num_first_options + num_second_options) * sizeof(QemuOptDesc));
(num_first_options + num_second_options + 1) since we assign
desc[index].name = NULL at the end.
Re: [Qemu-devel] [PATCH V5 09/10] Use QemuOpts support in block layer
On Thu, Nov 01, 2012 at 05:12:29PM +0800, Dong Xu Wang wrote:
> diff --git a/block/qcow.c b/block/qcow.c
> index b239c82..9bb736a 100644
> --- a/block/qcow.c
> +++ b/block/qcow.c
> @@ -651,7 +651,7 @@ static void qcow_close(BlockDriverState *bs)
> error_free(s->migration_blocker);
> }
>
> -static int qcow_create(const char *filename, QEMUOptionParameter *options)
> +static int qcow_create(const char *filename, QemuOpts *opts)
> {
> int header_size, backing_filename_len, l1_size, shift, i;
> QCowHeader header;
> @@ -662,19 +662,14 @@ static int qcow_create(const char *filename,
> QEMUOptionParameter *options)
> int ret;
> BlockDriverState *qcow_bs;
>
> -/* Read out options */
> -while (options && options->name) {
> -if (!strcmp(options->name, BLOCK_OPT_SIZE)) {
> -total_size = options->value.n / 512;
> -} else if (!strcmp(options->name, BLOCK_OPT_BACKING_FILE)) {
> -backing_file = options->value.s;
> -} else if (!strcmp(options->name, BLOCK_OPT_ENCRYPT)) {
> -flags |= options->value.n ? BLOCK_FLAG_ENCRYPT : 0;
> -}
> -options++;
> +/* Read out opts */
> +total_size = qemu_opt_get_number(opts, BLOCK_OPT_SIZE, 0) / 512;
> +backing_file = qemu_opt_get(opts, BLOCK_OPT_BACKING_FILE);
> +if (qemu_opt_get_bool(opts, BLOCK_OPT_ENCRYPT, 0)) {
> +flags |= BLOCK_FLAG_ENCRYPT;
> }
>
> -ret = bdrv_create_file(filename, options);
> +ret = bdrv_create_file(filename, opts);
Previously options would be an empty QEMUOptionParameter (because the
while loop iterated until options->name). Now you are passing the same
opts for creating this image file, this seems wrong. Should this be
bdrv_create_file(filename, NULL)?
> diff --git a/block/qcow2.c b/block/qcow2.c
> index c1ff31f..9cea051 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1180,7 +1180,7 @@ static int preallocate(BlockDriverState *bs)
> static int qcow2_create2(const char *filename, int64_t total_size,
> const char *backing_file, const char
> *backing_format,
> int flags, size_t cluster_size, int prealloc,
> - QEMUOptionParameter *options, int version)
> + QemuOpts *opts, int version)
The opts argument can be dropped since it is not used.
> @@ -1314,7 +1314,7 @@ out:
> return ret;
> }
>
> -static int qcow2_create(const char *filename, QEMUOptionParameter *options)
> +static int qcow2_create(const char *filename, QemuOpts *opts)
> {
> const char *backing_file = NULL;
> const char *backing_fmt = NULL;
> @@ -1323,45 +1323,41 @@ static int qcow2_create(const char *filename,
> QEMUOptionParameter *options)
> size_t cluster_size = DEFAULT_CLUSTER_SIZE;
> int prealloc = 0;
> int version = 2;
> +const char *buf;
>
> /* Read out options */
> -while (options && options->name) {
> -if (!strcmp(options->name, BLOCK_OPT_SIZE)) {
> -sectors = options->value.n / 512;
> -} else if (!strcmp(options->name, BLOCK_OPT_BACKING_FILE)) {
> -backing_file = options->value.s;
> -} else if (!strcmp(options->name, BLOCK_OPT_BACKING_FMT)) {
> -backing_fmt = options->value.s;
> -} else if (!strcmp(options->name, BLOCK_OPT_ENCRYPT)) {
> -flags |= options->value.n ? BLOCK_FLAG_ENCRYPT : 0;
> -} else if (!strcmp(options->name, BLOCK_OPT_CLUSTER_SIZE)) {
> -if (options->value.n) {
> -cluster_size = options->value.n;
> -}
> -} else if (!strcmp(options->name, BLOCK_OPT_PREALLOC)) {
> -if (!options->value.s || !strcmp(options->value.s, "off")) {
> -prealloc = 0;
> -} else if (!strcmp(options->value.s, "metadata")) {
> -prealloc = 1;
> -} else {
> -fprintf(stderr, "Invalid preallocation mode: '%s'\n",
> -options->value.s);
> -return -EINVAL;
> -}
> -} else if (!strcmp(options->name, BLOCK_OPT_COMPAT_LEVEL)) {
> -if (!options->value.s || !strcmp(options->value.s, "0.10")) {
> -version = 2;
> -} else if (!strcmp(options->value.s, "1.1")) {
> -version = 3;
> -} else {
> -fprintf(stderr, "Invalid compatibility level: '%s'\n",
> -options->value.s);
> -return -EINVAL;
> -}
> -} else if (!strcmp(options->name, BLOCK_OPT_LAZY_REFCOUNTS)) {
> -flags |= options->value.n ? BLOCK_FLAG_LAZY_REFCOUNTS : 0;
> -}
> -options++;
The old code worked when options == NULL. I think the new code should
also work when opts == NULL:
if (opts) {
}
This should be done for all block drivers or the way that
bdrv_create(..., NULL) works should be changed so that block dr
[Qemu-devel] [PATCH] qxl: Export "mode" as a property
This way you can easily tell from qemu if the guest is using
a qxl driver or not.
---
hw/qxl.c | 1 +
hw/qxl.h | 2 +-
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/hw/qxl.c b/hw/qxl.c
index 1d16863..98af0bb 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -2261,6 +2261,7 @@ static Property qxl_properties[] = {
DEFINE_PROP_UINT32("vram64_size_mb", PCIQXLDevice, vram_size_mb, -1),
DEFINE_PROP_UINT32("vgamem_mb", PCIQXLDevice, vgamem_size_mb, 16),
DEFINE_PROP_INT32("surfaces", PCIQXLDevice, ssd.num_surfaces, 1024),
+DEFINE_PROP_UINT32("mode", PCIQXLDevice, mode, 0),
DEFINE_PROP_END_OF_LIST(),
};
diff --git a/hw/qxl.h b/hw/qxl.h
index e583cfb..c4414ba 100644
--- a/hw/qxl.h
+++ b/hw/qxl.h
@@ -34,7 +34,7 @@ typedef struct PCIQXLDevice {
uint32_t guest_bug;
-enum qxl_mode mode;
+uint32_t mode;
uint32_t cmdflags;
intgeneration;
uint32_t revision;
--
1.7.12.1
[Qemu-devel] BlockDriverState member in_use (was: KVM Forum 2012 Block BoF minutes)
Marcelo Tosatti writes: > On Thu, Nov 15, 2012 at 02:31:53PM +0100, Markus Armbruster wrote: [...] >> BlockDriverState member in_use and DriveInfo member refcount are a mess: >> >> - in_use is used to avoid running certain things concurrently, but the >> rules are unclear, except they're clearly incomplete; possible rules >> could be about need for consistent views of image contents > > block: enable in_use flag > > Set block device in use during block migration, disallow drive_del and > bdrv_truncate for in use devices. > > 1) drive_del is not allowed because memory object used by block > streaming/migration can disappear. > 2) bdrv_truncate changes device size, which the block migration code was > unable to handle at the time. > > These are the rules (accordingly to the changeset). Yup. Your commits db593f25^..8591675f. > IIRC new rules have > been added (new uses for bdrv_in_use). Stefan's commit 2d3735d3. Let's have a look at the users of in_use (from notes I took back in August, possibly stale now): * bdrv_commit() Since commit 2d3735d3: block: check bdrv_in_use() before blockdev operations Long-running block operations like block migration and image streaming must have continual access to their block device. It is not safe to perform operations like hotplug, eject, change, resize, commit, or external snapshot while a long-running operation is in progress. This patch adds the missing bdrv_in_use() checks so that block migration and image streaming never have the rug pulled out from underneath them. Users are monitor command commit and terminal escape 's', directly and via bdrv_commit_all() What exactly would go wrong if someone commited during block migration / image streaming? Strange: also tests whether backing_hd is in_use. The other users don't, and I can't see how the test could ever succeed. Cock-up: bdrv_commit_all() stops on first error. * bdrv_truncate() Since commit 8591675f: block: enable in_use flag Set block device in use during block migration, disallow drive_del and bdrv_truncate for in use devices. Users are monitor command block_resize and a bunch of block drivers. I don't think block drivers are affected by in_use. They resize their internal BDSs such as bs->file, and I can't see how in_use could be set there. "No resize while block migration is running" is (was?) an artificial restriction; it should migrate the resize just like any other change. Image streaming, too, I guess. * block_job_create() Since commit eeec61f2: block: add BlockJob interface for long-running operations Use is monitor command block-stream. Why does the block job infrastructure set in_use? I suspect just because its only user "image streaming" needs it, but future users might not. If that's correct, it's set in the wrong place. * qmp_transaction() Since commit 2d3735d3 (see above). Function was called qmp_blockdev_snapshot_sync() then. User is monitor command snapshot_blkdev. What exactly would go wrong if someone snaphotted during block migration / image streaming? * eject_device() Since commit 2d3735d3 (see above). Users are monitor command eject, change. What would go wrong is obvious, for a change: we can't just kill the BDS while a long-running operation is using it. Could we just disassociate the BDS from the drive without killing it? So that when the job completes, the BDS's reference count drops to zero, and the BDS gets destroyed. * drive_del Since commit 8591675f (see above). In theory, in_use is unnecessary here: reference counting should delay the delete just fine. In practice, do_drive_del() *ignores* the reference count. Even if that was fixed, the count only delays drive_uninit(), not bdrv_drain_all()..bdrv_close(). Another fine mess. Note how we treat the device model's reference to the drive: if it exists, we make the drive anonymous instead of destroying it. It gets destroyed only when the device model drops the reference. Similar to reference counting. Tests of in_use are spread over block.c and monitor commands. Smells bad. If it was only about excluding monitor commands during certain long-running operations, the montor commands would be the appropriate place. Is it? > "Consistent views of image contents" is not defined. Imprecise language for "in_use seems to be about changes to the image, while refcount is for keeping objects alive". We were trying to make sense of the mess. > Question: why are the rules "clearly incomplete" ? Are we sure we catch everything that could interfere with block migration / image streaming? Why only these two? What about other long-running jobs? Do they need interference protection, too? Shouldn't there be clear rules on what changes can happen while a job runs, and which ones can be
[Qemu-devel] qemu-stable-1.1
Hello. I'm trying to pick whatever fixes applicable for 1.1.x series of qemu and qemu-kvm. Since Michael Roth said he will not be releasing more 1.1.x series, I think I can at least try to do that. Qemu-1.1 will be included in upcoming Debian Wheezy release, so we're interested in keeping it stable. I've a git tree and a branch which I created for this purpose, available at http://git.corpit.ru/?p=qemu.git;a=shortlog;h=refs/heads/stable-1.1-queue (git://git.corpit.ru/qemu.git#stable-1.1-queue ) -- this is the patches I picked from upstream git tree which - in my opinion - are applicable for 1.1.x. Please take a look. This is the first time I'm trying to do something like that, and am not sure if that's okay or not. Also, if there are any other fixes missing in there but should be, please ping me (or qemu-stable@). I plan to send out notifications about each change included there, in order to confirm that's okay. References to any scripts that may help there are definitely welcome. There are quite a few more fixes I'd add, especially in the USB front, but this area received too much changes past 1.1 version, including switching to asyncronous processing, so backporting stuff appears to be quite a bit difficult. Also, there are a few important changes which were posted to list (with pull requests too), but has not been merged for quite a while. Again, if there are some tools to help tracking such patches (like, to notify me on `git remote update' that some patch has been applied), please mention it, -- I'm trying to find something in this area. The git branch mentioned above is somewhat volatile, ie, I can rebase it at any time, when it becomes clear that some patch/change in there is not applicable for stable series. Many of the fixes in there are applicable for 1.2.x stable release too, but since Michael said he will be doing that series, I'm not tracking these. I can help here as well, if there's a need. Thank you for your attention. /mjt
[Qemu-devel] [PATCH] LICENSE: clarify licensing
There's a couple changes here. Please note that no one should consider this as licensing guidance from IBM. I'm merely updating this based on what I would consider community consensus. The first change clarifies that the overall license is GPLv2. We have never had GPLv1 code. The phrase "GNU General Public License" clearly refers to the GPLv2. We cannot say the overall license is "GPLv2 or later" because we have a number of files that are GPLv2 only. The second change clarifies that any file that doesn't have an explicit license is covered under the GPLv2. This agrees with the first statement in LICENSE but since it's so often asked, I thought we should make it explicit. I also added an additional statement that this file only expresses the intent of the QEMU community and should only be considered informative. Please explicitly Nack or Ack this change. Signed-off-by: Anthony Liguori --- LICENSE | 8 ++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/LICENSE b/LICENSE index acae9a3..f4bee4c 100644 --- a/LICENSE +++ b/LICENSE @@ -1,6 +1,6 @@ The following points clarify the QEMU license: -1) QEMU as a whole is released under the GNU General Public License +1) QEMU as a whole is released under the GNU General Public License version 2 2) Parts of QEMU have specific licenses which are compatible with the GNU General Public License. Hence each source file contains its own @@ -13,4 +13,8 @@ Many hardware device emulation sources are released under the BSD license. 4) QEMU is a trademark of Fabrice Bellard. -Fabrice Bellard. +5) Files without explicit licenses fall under the GPL v2. + +6) This file represents the consensus view of the QEMU community. It does not + alter any licenses used in the code base and should be viewed as informative + only. -- 1.8.0
Re: [Qemu-devel] [0/12] Pending pseries patches
On 13.11.2012, at 03:46, David Gibson wrote: > Here again is my current set of outstanding pseries patches, updated > for current upstream. I don't think any of these has changed in > substance since their last posting. As explained last time around, > some of the From/Signed-off-by combinations are a bit odd, but I think > as accurate reflection of the patch's history as is possible. > > Please apply. Thanks, applied all except for: 3,8,12 to ppc-next. Is any of these patches urgent for the 1.3 release? Alex
Re: [Qemu-devel] [PATCH 12/12] pseries: Generate unique LIOBNs for PCI host bridges
On 13.11.2012, at 03:47, David Gibson wrote:
> From: Alexey Kardashevskiy
>
> In future (with VFIO) we will have multiple PCI host bridges on
> pseries. Each one needs a unique LIOBN (IOMMU id). At the moment we
> derive these from the pci domain number, but the whole notion of
> domain numbers on the qemu side is bogus and in any case they're not
> actually uniquely allocated at this point.
>
> This patch, therefore uses a simple sequence counter to generate
> unique LIOBNs for PCI host bridges.
>
> Signed-off-by: Alexey Kardashevskiy
> Signed-off-by: David Gibson
I don't really like the idea of having a global variable just because our
domain ID generation seems to not work as expected. Michael, any comments here?
Alex
> ---
> hw/spapr_pci.c |3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/hw/spapr_pci.c b/hw/spapr_pci.c
> index 3c5b855..f6544d7 100644
> --- a/hw/spapr_pci.c
> +++ b/hw/spapr_pci.c
> @@ -521,6 +521,7 @@ static int spapr_phb_init(SysBusDevice *s)
> char *namebuf;
> int i;
> PCIBus *bus;
> +static int phbnum;
>
> sphb->dtbusname = g_strdup_printf("pci@%" PRIx64, sphb->buid);
> namebuf = alloca(strlen(sphb->dtbusname) + 32);
> @@ -572,7 +573,7 @@ static int spapr_phb_init(SysBusDevice *s)
>PCI_DEVFN(0, 0), PCI_NUM_PINS);
> phb->bus = bus;
>
> -sphb->dma_liobn = SPAPR_PCI_BASE_LIOBN | (pci_find_domain(bus) << 16);
> +sphb->dma_liobn = SPAPR_PCI_BASE_LIOBN | (++phbnum << 16);
> sphb->dma_window_start = 0;
> sphb->dma_window_size = 0x4000;
> sphb->dma = spapr_tce_new_dma_context(sphb->dma_liobn,
> sphb->dma_window_size);
> --
> 1.7.10.4
>
Re: [Qemu-devel] [PATCH] block: add bdrv_reopen() support for raw hdev devices
On 11/19/2012 10:45 AM, Kevin Wolf wrote: > Am 19.11.2012 16:37, schrieb Jeff Cody: >> The host device reopen handler for raw-posix is the same as the file reopen >> handler. >> > > What about host_floppy and (twice) host_cdrom? > > Kevin > Do we need / want reopen for floppy and/or cdrom?
Re: [Qemu-devel] [PATCH 3/5] exec: Do not use absolute address hints for code_gen_buffer with -fpie
On 2012-11-18 12:48, Stefan Weil wrote: > This patch breaks the TCG interpreter. Here is a test run on Debian x86_64 > (output shortened): Nack. This is hiding some bug elsewhere in the tcg interpreter. I disbelieve that the interpreter *requires* a pointer in the low 32-bits of the x86_64 address space. r~
Re: [Qemu-devel] [PATCH 08/12] target-ppc: Convert ppcemb_tlb_t to use fixed 64-bit RPN
On 13.11.2012, at 03:46, David Gibson wrote:
> Currently the ppcemb_tlb_t struct, used on a number of embedded ppc models
> to represent a TLB entry contains a hwaddr. That works reasonably for now,
> but is troublesome for saving the state, which we'll want to do in future.
> hwaddr is a large enough type to contain a physical address for any
> supported machine - and can thus, in theory at least, vary depending on
> what machines are enabled other than the one we're actually using right
> now (though in fact it doesn't for ppc). This makes it unsuitable for
> describing in vmstate.
>
> This patch therefore changes ppcemb_tlb_t to use a fixed 64-bit integer
> which we know is sufficient for all the machines which use this structure.
hwaddr is always defined to 64bit by now.
Alex
>
> Signed-off-by: David Gibson
> ---
> target-ppc/cpu.h |2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
> index 5f1dc8b..742d4f8 100644
> --- a/target-ppc/cpu.h
> +++ b/target-ppc/cpu.h
> @@ -355,7 +355,7 @@ struct ppc6xx_tlb_t {
>
> typedef struct ppcemb_tlb_t ppcemb_tlb_t;
> struct ppcemb_tlb_t {
> -hwaddr RPN;
> +uint64_t RPN;
> target_ulong EPN;
> target_ulong PID;
> target_ulong size;
> --
> 1.7.10.4
>
[Qemu-devel] [PATCH] migration: fix rate limiting
buffered_rate_limit is called to prevent the RAM migration callback from putting too much data in the buffer. So it has to check against the amount of data currently in the buffer, not against the amount of data that has been transferred so far. s->bytes_xfer is used to communicate between successive calls of buffered_put_buffer. Buffered_rate_tick resets it every now and then to prevent moving too much buffered data to the socket at once. However, its value does not matter for the producer of the data. Here is the result for migrating an idle guest with 3GB of memory and ~360MB of non-zero memory: migrate_set_speed BeforeAfter default (32MB/sec) ~3 sec~13 sec infinite (10GB/sec) ~3 sec~3 sec Note that before this patch, QEMU is transferring of 100 MB/sec despite the rate limiting. Signed-off-by: Paolo Bonzini --- buffered_file.c | 2 +- 1 file modificato, 1 inserzione(+). 1 rimozione(-) diff --git a/buffered_file.c b/buffered_file.c index bd0f61d..46cd591 100644 --- a/buffered_file.c +++ b/buffered_file.c @@ -193,7 +193,7 @@ static int buffered_rate_limit(void *opaque) if (s->freeze_output) return 1; -if (s->bytes_xfer > s->xfer_limit) +if (s->buffer_size > s->xfer_limit) return 1; return 0; -- 1.7.12.1
Re: [Qemu-devel] [PATCH 03/12] pseries: Move XICS initialization before cpu initialization
On 13.11.2012, at 03:46, David Gibson wrote:
> From: Ben Herrenschmidt
>
> Currently, the pseries machine initializes the cpus, then the XICS
> interrupt controller. However, to support the upcoming in-kernel XICS
> implementation we will need to initialize the irq controller before the
> vcpus. This patch makes the necesssary rearrangement. This means the
> xics init code can no longer auto-detect the number of cpus ("interrupt
> servers" in XICS terminology) and so we must pass that in explicitly from
> the platform code.
Does this still hold true with the new in-kernel interrupt controller workflow?
Alex
>
> Signed-off-by: Michael Ellerman
> Signed-off-by: Ben Herrenschmidt
> Signed-off-by: David Gibson
> ---
> hw/spapr.c | 12 +++-
> hw/xics.c | 52 +++-
> hw/xics.h |3 ++-
> 3 files changed, 32 insertions(+), 35 deletions(-)
>
> diff --git a/hw/spapr.c b/hw/spapr.c
> index eafee03..dc2349c 100644
> --- a/hw/spapr.c
> +++ b/hw/spapr.c
> @@ -747,6 +747,11 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
> spapr->htab_shift++;
> }
>
> +/* Set up Interrupt Controller before we create the VCPUs */
> +spapr->icp = xics_system_init(smp_cpus * kvmppc_smt_threads() /
> smp_threads,
> + XICS_IRQS);
> +spapr->next_irq = XICS_IRQ_BASE;
> +
> /* init CPUs */
> if (cpu_model == NULL) {
> cpu_model = kvm_enabled() ? "host" : "POWER7";
> @@ -759,6 +764,8 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
> }
> env = &cpu->env;
>
> +xics_cpu_setup(spapr->icp, env);
> +
> /* Set time-base frequency to 512 MHz */
> cpu_ppc_tb_init(env, TIMEBASE_FREQ);
>
> @@ -798,11 +805,6 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
> }
> g_free(filename);
>
> -
> -/* Set up Interrupt Controller */
> -spapr->icp = xics_system_init(XICS_IRQS);
> -spapr->next_irq = XICS_IRQ_BASE;
> -
> /* Set up EPOW events infrastructure */
> spapr_events_init(spapr);
>
> diff --git a/hw/xics.c b/hw/xics.c
> index b8887cd..f72dfae 100644
> --- a/hw/xics.c
> +++ b/hw/xics.c
> @@ -510,42 +510,36 @@ static void xics_reset(void *opaque)
> }
> }
>
> -struct icp_state *xics_system_init(int nr_irqs)
> +void xics_cpu_setup(struct icp_state *icp, CPUPPCState *env)
> {
> -CPUPPCState *env;
> -int max_server_num;
> -struct icp_state *icp;
> -struct ics_state *ics;
> +struct icp_server_state *ss = &icp->ss[env->cpu_index];
>
> -max_server_num = -1;
> -for (env = first_cpu; env != NULL; env = env->next_cpu) {
> -if (env->cpu_index > max_server_num) {
> -max_server_num = env->cpu_index;
> -}
> -}
> +assert(env->cpu_index < icp->nr_servers);
>
> -icp = g_malloc0(sizeof(*icp));
> -icp->nr_servers = max_server_num + 1;
> -icp->ss = g_malloc0(icp->nr_servers*sizeof(struct icp_server_state));
> +switch (PPC_INPUT(env)) {
> +case PPC_FLAGS_INPUT_POWER7:
> +ss->output = env->irq_inputs[POWER7_INPUT_INT];
> +break;
>
> -for (env = first_cpu; env != NULL; env = env->next_cpu) {
> -struct icp_server_state *ss = &icp->ss[env->cpu_index];
> +case PPC_FLAGS_INPUT_970:
> +ss->output = env->irq_inputs[PPC970_INPUT_INT];
> +break;
>
> -switch (PPC_INPUT(env)) {
> -case PPC_FLAGS_INPUT_POWER7:
> -ss->output = env->irq_inputs[POWER7_INPUT_INT];
> -break;
> +default:
> +fprintf(stderr, "XICS interrupt controller does not support this CPU
> "
> +"bus model\n");
> +abort();
> +}
> +}
>
> -case PPC_FLAGS_INPUT_970:
> -ss->output = env->irq_inputs[PPC970_INPUT_INT];
> -break;
> +struct icp_state *xics_system_init(int nr_servers, int nr_irqs)
> +{
> +struct icp_state *icp;
> +struct ics_state *ics;
>
> -default:
> -hw_error("XICS interrupt model does not support this CPU bus "
> - "model\n");
> -exit(1);
> -}
> -}
> +icp = g_malloc0(sizeof(*icp));
> +icp->nr_servers = nr_servers;
> +icp->ss = g_malloc0(icp->nr_servers*sizeof(struct icp_server_state));
>
> ics = g_malloc0(sizeof(*ics));
> ics->nr_irqs = nr_irqs;
> diff --git a/hw/xics.h b/hw/xics.h
> index c3bf008..b43678a 100644
> --- a/hw/xics.h
> +++ b/hw/xics.h
> @@ -35,6 +35,7 @@ struct icp_state;
> qemu_irq xics_get_qirq(struct icp_state *icp, int irq);
> void xics_set_irq_type(struct icp_state *icp, int irq, bool lsi);
>
> -struct icp_state *xics_system_init(int nr_irqs);
> +struct icp_state *xics_system_init(int nr_servers, int nr_irqs);
> +void xics_cpu_setup(struct icp_state *icp, CPUPPCState *env);
>
> #endif /* __XICS_H__ */
> --
> 1.7.10.4
>
Re: [Qemu-devel] [PATCH] block: add bdrv_reopen() support for raw hdev devices
Am 19.11.2012 17:16, schrieb Jeff Cody: > On 11/19/2012 10:45 AM, Kevin Wolf wrote: >> Am 19.11.2012 16:37, schrieb Jeff Cody: >>> The host device reopen handler for raw-posix is the same as the file reopen >>> handler. >>> > >> >> What about host_floppy and (twice) host_cdrom? >> >> Kevin >> > > Do we need / want reopen for floppy and/or cdrom? At least I can't see what makes it less reasonable than for host_device. They are really just host_device with one or two additional callbacks for handling removable media. Kevin
Re: [Qemu-devel] TCP based PCIE request forwarding
Hi, Thanks, it is actually a good idea to start with. I will write a spec based on an improved version of what I have already implemented. I think I will have some time this week, I will keep you updated soon. Best regards, Fabien. 2012/11/19 Stefan Hajnoczi : > On Fri, Nov 16, 2012 at 02:05:29PM +0100, lementec fabien wrote: >> Actually, I wanted to be independant of the QEMU event loop. Plus, >> some proprietary simulation environment provides a closed socket >> based interface to 'stimulate' the emulated device, at the PCIE level >> for instance. These environments are sometimes installed on cluster >> not running QEMU. The socket based approach fits quite well. >> >> Not knowing about QEMU internals, I spent some hours trying to find >> out the best way to plug into QEMU, and did not find ivhsmem appropriate. >> Honestly, I wanted to have a working solution asap, and it did not take >> long before I opted for the socket based approach. Now that it is working, >> I can take time to reconsider stuffs according to others need, and ideally >> an integration to QEMU. > > I suggest writing up a spec for the socket protocol. It can be put in > docs/specs/ (like the ivshmem spec). > > This is both a good way to increase discussion and important for others > who may wish to make use of this feature. > > Stefan
Re: [Qemu-devel] scsi-hd with discard_granularity and unmap results in Aborted Commands
Am 19.11.2012 16:22, schrieb Paolo Bonzini:
Il 19/11/2012 16:04, Stefan Priebe - Profihost AG ha scritto:
[ 49.183366] sd 2:0:0:1: [sdb]
[ 49.183366] Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE
[ 49.183366] sd 2:0:0:1: [sdb]
[ 49.183366] Sense Key : Aborted Command [current]
[ 49.183366] sd 2:0:0:1: [sdb]
[ 49.183366] Add. Sense: I/O process terminated
[ 49.183366] sd 2:0:0:1: [sdb] CDB:
[ 49.183366] Write same(16): 93 08 00 00 00 00 03 ff ff f8 00 7f ff ff
00 00
[ 49.183366] end_request: I/O error, dev sdb, sector 67108856
[ 49.183366] sd 2:0:0:1: [sdb]
[ 49.183366] Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE
[ 49.183366] sd 2:0:0:1: [sdb]
[ 49.183366] Sense Key : Aborted Command [current]
[ 49.183366] sd 2:0:0:1: [sdb]
[ 49.183366] Add. Sense: I/O process terminated
[ 49.183366] sd 2:0:0:1: [sdb] CDB:
[ 49.183366] Write same(16): 93 08 00 00 00 00 04 7f ff f7 00 62 00 09
00 00
[ 49.183366] end_request: I/O error, dev sdb, sector 75497463
This is not a cancel. It happens when the block layer reports an error.
You can try adding a printf to rbd_aio_bh_cb, like "if (acb->ret < 0)
printf("error... %d\n", acb->ret);".
Yes this one get's interesting values back:
rbd_aio_bh_cb got error back. acb->ret: -512 acb->error: 0
rbd_aio_bh_cb got error back. acb->ret: -512 acb->error: 0
rbd_aio_bh_cb got error back. acb->ret: -512 acb->error: 0
rbd_aio_bh_cb got error back. acb->ret: -512 acb->error: 0
rbd_aio_bh_cb got error back. acb->ret: -512 acb->error: 0
rbd_aio_bh_cb got error back. acb->ret: -512 acb->error: 0
rbd_aio_bh_cb got error back. acb->ret: -512 acb->error: 0
rbd_aio_bh_cb got error back. acb->ret: -512 acb->error: 0
rbd_aio_bh_cb got error back. acb->ret: -512 acb->error: 0
rbd_aio_bh_cb got error back. acb->ret: -1006628352 acb->error: 0
Stefan
Re: [Qemu-devel] [PATCH] iscsi: partly avoid iovec linearization in iscsi_aio_writev
On Mon, Nov 19, 2012 at 7:18 AM, Paolo Bonzini wrote:
> Il 19/11/2012 15:58, Peter Lieven ha scritto:
>>
>> -/* XXX we should pass the iovec to write16 to avoid the extra copy */
>> -/* this will allow us to get rid of 'buf' completely */
>> size = nb_sectors * BDRV_SECTOR_SIZE;
>> -acb->buf = g_malloc(size);
>> -qemu_iovec_to_buf(acb->qiov, 0, acb->buf, size);
>> +data.size = size;
>> +
>> +/* if the iovec only contains one buffer we can pass it directly */
>> +if (acb->qiov->niov == 1) {
>> +acb->buf = NULL;
>> +data.data = acb->qiov->iov[0].iov_base;
>> +} else {
>> +acb->buf = g_malloc(size);
>> +qemu_iovec_to_buf(acb->qiov, 0, acb->buf, size);
>> +data.data = acb->buf;
>> +}
>
> Looks good, but how hard is it to get rid of the bounce buffer
> completely, as mentioned in the comment? Ronnie?
>
I am working on that, but I will need the thanksgiving weekend to
finish it and test it.
It also means breaking the API, since it would introduce new
functionality so it will take a little while after finishing the
libiscsi part before we could/should introduce it in qemu.
The vast majority of writes seems to be a vector with only a single
element, so Peters change is a good optimization for the common case,
until I get the proper fix finished and tested and pushed in a new
release of libiscsi.
I.e. I think Peters change is good for now. It solves the problem
with "extra memcpy" for the majority of writes until we can get the
full solution ready.
> Paolo
Re: [Qemu-devel] [PATCH] block: add bdrv_reopen() support for raw hdev devices
Am 19.11.2012 16:37, schrieb Jeff Cody:
> The host device reopen handler for raw-posix is the same as the file reopen
> handler.
>
> Signed-off-by: Jeff Cody
> ---
> block/raw-posix.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index f2f0404..c7061e6 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -1409,6 +1409,9 @@ static BlockDriver bdrv_host_device = {
> .bdrv_probe_device = hdev_probe_device,
> .bdrv_file_open = hdev_open,
> .bdrv_close = raw_close,
> +.bdrv_reopen_prepare = raw_reopen_prepare,
> +.bdrv_reopen_commit = raw_reopen_commit,
> +.bdrv_reopen_abort = raw_reopen_abort,
> .bdrv_create= hdev_create,
> .create_options = raw_create_options,
> .bdrv_has_zero_init = hdev_has_zero_init,
>
What about host_floppy and (twice) host_cdrom?
Kevin
[Qemu-devel] [PATCH] block: add bdrv_reopen() support for raw hdev devices
The host device reopen handler for raw-posix is the same as the file reopen
handler.
Signed-off-by: Jeff Cody
---
block/raw-posix.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/block/raw-posix.c b/block/raw-posix.c
index f2f0404..c7061e6 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -1409,6 +1409,9 @@ static BlockDriver bdrv_host_device = {
.bdrv_probe_device = hdev_probe_device,
.bdrv_file_open = hdev_open,
.bdrv_close = raw_close,
+.bdrv_reopen_prepare = raw_reopen_prepare,
+.bdrv_reopen_commit = raw_reopen_commit,
+.bdrv_reopen_abort = raw_reopen_abort,
.bdrv_create= hdev_create,
.create_options = raw_create_options,
.bdrv_has_zero_init = hdev_has_zero_init,
--
1.7.11.7
Re: [Qemu-devel] [PATCH 1/3] hd-geometry.c: Integrate HDIO_GETGEO in guessing
On 19/11/12 16:08, Markus Armbruster wrote: > You're undoing my work separation of guessing L-CHS from the DOS > partition table and choosing a translation. Why? Might be my fault during rebasing. Will check with Einar.
Re: [Qemu-devel] [PATCH v5 5/6] i8259: fix so that dropping IRQ level always clears the interrupt request
On Sun, 9 Sep 2012, Matthew Ogilvie wrote:
Intel's definition of "edge triggered" means: "asserted with a
low-to-high transition at the time an interrupt is registered and
then kept high until the interrupt is served via one of the
EOI mechanisms or goes away unhandled."
So the only difference between edge triggered and level triggered
is in the leading edge, with no difference in the trailing edge.
This bug manifested itself when the guest was Microport UNIX
System V/386 v2.1 (ca. 1987), because it would sometimes mask
off IRQ14 in the slave IMR after it had already been asserted.
The master would still try to deliver an interrupt even though
IRQ2 had dropped again, resulting in a spurious interupt
(IRQ15) and a panicked kernel.
Signed-off-by: Matthew Ogilvie
---
If you missed the previous thread about this, see
http://www.mail-archive.com/qemu-devel@nongnu.org/msg129071.html
hw/i8259.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/hw/i8259.c b/hw/i8259.c
index 6587666..c011787 100644
--- a/hw/i8259.c
+++ b/hw/i8259.c
@@ -157,6 +157,7 @@ static void pic_set_irq(void *opaque, int irq, int level)
}
s->last_irr |= mask;
} else {
+s->irr &= ~mask;
s->last_irr &= ~mask;
}
}
What happened to this patch? Any chance it will be in 1.3?
Thanks,
BALATON Zoltan
Re: [Qemu-devel] [PATCH] iscsi: partly avoid iovec linearization in iscsi_aio_writev
Am 19.11.2012 um 16:18 schrieb Paolo Bonzini :
> Il 19/11/2012 15:58, Peter Lieven ha scritto:
>>
>> -/* XXX we should pass the iovec to write16 to avoid the extra copy */
>> -/* this will allow us to get rid of 'buf' completely */
>> size = nb_sectors * BDRV_SECTOR_SIZE;
>> -acb->buf = g_malloc(size);
>> -qemu_iovec_to_buf(acb->qiov, 0, acb->buf, size);
>> +data.size = size;
>> +
>> +/* if the iovec only contains one buffer we can pass it directly */
>> +if (acb->qiov->niov == 1) {
>> +acb->buf = NULL;
>> +data.data = acb->qiov->iov[0].iov_base;
>> +} else {
>> +acb->buf = g_malloc(size);
>> +qemu_iovec_to_buf(acb->qiov, 0, acb->buf, size);
>> +data.data = acb->buf;
>> +}
>
> Looks good, but how hard is it to get rid of the bounce buffer
> completely, as mentioned in the comment? Ronnie?
afaik, he is working on that. but therefore it needs support for passing
buffers to libiscsi also for write operations (currently thats only possible
for reads).
this was just an easy catch and can be reverted once libiscsi has support
for this. we are working on some abi changes which separate libiscsi
for the scsi-lowlevel operations. we have to make modifications to the
qemu driver then as well, maybe this could be done at the same point.
Peter
>
> Paolo
Re: [Qemu-devel] scsi-hd with discard_granularity and unmap results in Aborted Commands
Il 19/11/2012 16:04, Stefan Priebe - Profihost AG ha scritto:
> [ 49.183366] sd 2:0:0:1: [sdb]
> [ 49.183366] Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE
> [ 49.183366] sd 2:0:0:1: [sdb]
> [ 49.183366] Sense Key : Aborted Command [current]
> [ 49.183366] sd 2:0:0:1: [sdb]
> [ 49.183366] Add. Sense: I/O process terminated
> [ 49.183366] sd 2:0:0:1: [sdb] CDB:
> [ 49.183366] Write same(16): 93 08 00 00 00 00 03 ff ff f8 00 7f ff ff
> 00 00
> [ 49.183366] end_request: I/O error, dev sdb, sector 67108856
> [ 49.183366] sd 2:0:0:1: [sdb]
> [ 49.183366] Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE
> [ 49.183366] sd 2:0:0:1: [sdb]
> [ 49.183366] Sense Key : Aborted Command [current]
> [ 49.183366] sd 2:0:0:1: [sdb]
> [ 49.183366] Add. Sense: I/O process terminated
> [ 49.183366] sd 2:0:0:1: [sdb] CDB:
> [ 49.183366] Write same(16): 93 08 00 00 00 00 04 7f ff f7 00 62 00 09
> 00 00
> [ 49.183366] end_request: I/O error, dev sdb, sector 75497463
This is not a cancel. It happens when the block layer reports an error.
You can try adding a printf to rbd_aio_bh_cb, like "if (acb->ret < 0)
printf("error... %d\n", acb->ret);".
Paolo
Re: [Qemu-devel] [PATCH] iscsi: partly avoid iovec linearization in iscsi_aio_writev
Il 19/11/2012 15:58, Peter Lieven ha scritto:
>
> -/* XXX we should pass the iovec to write16 to avoid the extra copy */
> -/* this will allow us to get rid of 'buf' completely */
> size = nb_sectors * BDRV_SECTOR_SIZE;
> -acb->buf = g_malloc(size);
> -qemu_iovec_to_buf(acb->qiov, 0, acb->buf, size);
> +data.size = size;
> +
> +/* if the iovec only contains one buffer we can pass it directly */
> +if (acb->qiov->niov == 1) {
> +acb->buf = NULL;
> +data.data = acb->qiov->iov[0].iov_base;
> +} else {
> +acb->buf = g_malloc(size);
> +qemu_iovec_to_buf(acb->qiov, 0, acb->buf, size);
> +data.data = acb->buf;
> +}
Looks good, but how hard is it to get rid of the bounce buffer
completely, as mentioned in the comment? Ronnie?
Paolo
Re: [Qemu-devel] [PATCH 1/3] hd-geometry.c: Integrate HDIO_GETGEO in guessing
Christian Borntraeger writes:
> From: Einar Lueck
>
> This patch extends the function guess_disk_lchs. If no geo could
> be derived from reading disk content, HDIO_GETGEO ioctl is issued.
> If this is not successful (e.g. image files) geometry is derived
> from the size of the disk (as before). To achieve this, the MSDOS
> partition table reading logic and the translation computation logic
> have been moved into a separate function guess_disk_msdosgeo. The
> HDIO_GETGEO logic is captured in a new function guess_disk_ioctlgeo.
> guess_disk_lchs then encapsulates both (the overall guessing logic).
> The new HDIO_GETGEO logic is required for two use cases:
> a) Support for geometries of Direct Attached Storage Disks (DASD)
> on s390x configured as backing of virtio block devices.
> b) Support for FCP attached SCSI disks that do not yet have a
> partition table. Without this patch, fdisk -l on the host would
> return different results then fdisk -l in the guest.
>
> Signed-off-by: Einar Lueck
> Signed-off-by: Jens Freimann
> Reviewed-by: Carsten Otte
> Signed-off-by: Christian Borntraeger
> ---
> hw/hd-geometry.c | 124
> ++
> 1 file changed, 87 insertions(+), 37 deletions(-)
>
> diff --git a/hw/hd-geometry.c b/hw/hd-geometry.c
> index 1cdb9fb..4cf040d 100644
> --- a/hw/hd-geometry.c
> +++ b/hw/hd-geometry.c
> @@ -33,6 +33,10 @@
> #include "block.h"
> #include "hw/block-common.h"
> #include "trace.h"
> +#ifdef __linux__
> +#include
> +#include
> +#endif
>
> struct partition {
> uint8_t boot_ind; /* 0x80 - active */
> @@ -47,13 +51,65 @@ struct partition {
> uint32_t nr_sects; /* nr of sectors in partition */
> } QEMU_PACKED;
>
> +static void guess_chs_for_size(BlockDriverState *bs,
> +uint32_t *pcyls, uint32_t *pheads, uint32_t *psecs)
> +{
> +uint64_t nb_sectors;
> +int cylinders;
> +
> +bdrv_get_geometry(bs, &nb_sectors);
> +
> +cylinders = nb_sectors / (16 * 63);
> +if (cylinders > 16383) {
> +cylinders = 16383;
> +} else if (cylinders < 2) {
> +cylinders = 2;
> +}
> +*pcyls = cylinders;
> +*pheads = 16;
> +*psecs = 63;
> +}
Code motion, because you're reordering functions. Best avoided, unless
it really helps readability.
> +
> +/* try to get geometry from disk via HDIO_GETGEO ioctl
> + Return 0 if OK, -1 if ioctl does not work (e.g. image file) */
> +static inline int guess_disk_ioctlgeo(BlockDriverState *bs,
> + uint32_t *pcylinders, uint32_t *pheads,
> + uint32_t *psectors, int *ptranslation)
> +{
> +#ifdef __linux__
> +struct hd_geometry geo;
> +
> +if (bdrv_ioctl(bs, HDIO_GETGEO, &geo)) {
> +return -1;
> +}
> +
> +/* HDIO_GETGEO may return success even though geo contains zeros
> + (e.g. certain multipath setups) */
> +if (!geo.heads || !geo.sectors || !geo.cylinders) {
> +return -1;
> +}
> +
> +*pheads = geo.heads;
> +*psectors = geo.sectors;
> +*pcylinders = geo.cylinders;
> +*ptranslation = BIOS_ATA_TRANSLATION_NONE;
> +trace_hd_geometry_lchs_guess(bs, *pcylinders, *pheads, *psectors);
Abuse of existing trace point. Please define a new one!
> +return 0;
> +#else
> +return -1;
> +#endif
> +}
> +
> +
> /* try to guess the disk logical geometry from the MSDOS partition table.
> Return 0 if OK, -1 if could not guess */
> -static int guess_disk_lchs(BlockDriverState *bs,
> - int *pcylinders, int *pheads, int *psectors)
> +static int guess_disk_msdosgeo(BlockDriverState *bs,
> + uint32_t *pcylinders, uint32_t *pheads,
> + uint32_t *psectors, int *ptranslation)
I don't like your new name.
There are two kinds of geometry in the DOS world:
* Physical: property of the disk, used for CHS addressing of disk
sectors when communicating with the disk, say via ATA. Obsolete in
ATA, superseded by LBA. This is the common meaning of geometry.
* Logical: something the PC BIOS makes up to make the best use of its
ill-designed int 13h CHS addressing. Used only by really old software
like DOS.
The old function name makes it explicit that this function is about
*logical* CHS, unlike the other functions in this file.
I'd be fine with _dos_lchs, or _pc_lchs, or leaving it as _lchs.
> {
> uint8_t buf[BDRV_SECTOR_SIZE];
> -int i, heads, sectors, cylinders;
> +int i, translation;
> +uint32_t heads, sectors, cylinders;
> struct partition *p;
> uint32_t nr_sects;
> uint64_t nb_sectors;
> @@ -87,9 +143,26 @@ static int guess_disk_lchs(BlockDriverState *bs,
> if (cylinders < 1 || cylinders > 16383) {
> continue;
> }
> +
> +if (heads > 16) {
> +/* LCHS guess with heads > 16 means that a BIOS L
Re: [Qemu-devel] scsi-hd with discard_granularity and unmap results in Aborted Commands
Hi Paolo,
new patch attached. Desciption is still wrong.
> I think this is all unneeded. Just store rcb->ret into
> rcb->acb->status, and your version of qemu_rbd_aio_cancel should just
> work.
>
> Also, I think the acb->cancelled field is not necessary anymore after
> these changes.
1.) It removes cancelled
2.) It adds status variable
3.) aio cancel now just waits for io completetion
This should fix the write race you mentioned. But it still does not help
with discard the kernel starts to cancel as the reply takes too long. See:
[ 49.183366] sd 2:0:0:1: [sdb]
[ 49.183366] Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE
[ 49.183366] sd 2:0:0:1: [sdb]
[ 49.183366] Sense Key : Aborted Command [current]
[ 49.183366] sd 2:0:0:1: [sdb]
[ 49.183366] Add. Sense: I/O process terminated
[ 49.183366] sd 2:0:0:1: [sdb] CDB:
[ 49.183366] Write same(16): 93 08 00 00 00 00 03 ff ff f8 00 7f ff ff
00 00
[ 49.183366] end_request: I/O error, dev sdb, sector 67108856
[ 49.183366] sd 2:0:0:1: [sdb]
[ 49.183366] Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE
[ 49.183366] sd 2:0:0:1: [sdb]
[ 49.183366] Sense Key : Aborted Command [current]
[ 49.183366] sd 2:0:0:1: [sdb]
[ 49.183366] Add. Sense: I/O process terminated
[ 49.183366] sd 2:0:0:1: [sdb] CDB:
[ 49.183366] Write same(16): 93 08 00 00 00 00 04 7f ff f7 00 62 00 09
00 00
[ 49.183366] end_request: I/O error, dev sdb, sector 75497463
Greets,
Stefan
>From d65f2c2ba8c81842992953dd772355898e702968 Mon Sep 17 00:00:00 2001
From: Stefan Priebe
Date: Mon, 19 Nov 2012 15:54:05 +0100
Subject: [PATCH] fix cancel rbd race
Signed-off-by: Stefan Priebe
---
block/rbd.c | 19 ---
1 file changed, 12 insertions(+), 7 deletions(-)
diff --git a/block/rbd.c b/block/rbd.c
index 5a0f79f..7b3bcbb 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -76,7 +76,7 @@ typedef struct RBDAIOCB {
int64_t sector_num;
int error;
struct BDRVRBDState *s;
-int cancelled;
+int status;
} RBDAIOCB;
typedef struct RADOSCB {
@@ -376,9 +376,7 @@ static void qemu_rbd_complete_aio(RADOSCB *rcb)
RBDAIOCB *acb = rcb->acb;
int64_t r;
-if (acb->cancelled) {
-qemu_vfree(acb->bounce);
-qemu_aio_release(acb);
+if (acb->bh) {
goto done;
}
@@ -406,9 +404,12 @@ static void qemu_rbd_complete_aio(RADOSCB *rcb)
acb->ret = r;
}
}
+acb->status = acb->ret;
+
/* Note that acb->bh can be NULL in case where the aio was cancelled */
acb->bh = qemu_bh_new(rbd_aio_bh_cb, acb);
qemu_bh_schedule(acb->bh);
+
done:
g_free(rcb);
}
@@ -573,7 +574,10 @@ static void qemu_rbd_close(BlockDriverState *bs)
static void qemu_rbd_aio_cancel(BlockDriverAIOCB *blockacb)
{
RBDAIOCB *acb = (RBDAIOCB *) blockacb;
-acb->cancelled = 1;
+
+while (acb->status == -EINPROGRESS) {
+qemu_aio_wait();
+}
}
static AIOPool rbd_aio_pool = {
@@ -642,10 +646,11 @@ static void rbd_aio_bh_cb(void *opaque)
qemu_iovec_from_buf(acb->qiov, 0, acb->bounce, acb->qiov->size);
}
qemu_vfree(acb->bounce);
-acb->common.cb(acb->common.opaque, (acb->ret > 0 ? 0 : acb->ret));
qemu_bh_delete(acb->bh);
acb->bh = NULL;
+acb->common.cb(acb->common.opaque, (acb->ret > 0 ? 0 : acb->ret));
+
qemu_aio_release(acb);
}
@@ -689,8 +694,8 @@ static BlockDriverAIOCB *rbd_start_aio(BlockDriverState *bs,
acb->ret = 0;
acb->error = 0;
acb->s = s;
-acb->cancelled = 0;
acb->bh = NULL;
+acb->status = -EINPROGRESS;
if (cmd == RBD_AIO_WRITE) {
qemu_iovec_to_buf(acb->qiov, 0, acb->bounce, qiov->size);
--
1.7.10.4
Re: [Qemu-devel] scsi-hd with discard_granularity and unmap results in Aborted Commands
Il 19/11/2012 15:48, Stefan Priebe - Profihost AG ha scritto: > So you mean in qemu_rbd_complete_aio i should remove the check for > cancelled and then just overwrite acb->status to that it changes from > -EINPROGRESS to something else? > > And qemu_rbd_aio_cancel should just wait for status != -EINPROGRESS? Yes. paolo
[Qemu-devel] [PATCH] iscsi: partly avoid iovec linearization in iscsi_aio_writev
libiscsi expects all write16 data in a linear buffer. If the
iovec only contains one buffer we can skip the linearization
step as well as the additional malloc/free and pass the
buffer directly.
Reported-by: Ronnie Sahlberg
Signed-off-by: Peter Lieven
---
block/iscsi.c | 24 +++-
1 file changed, 15 insertions(+), 9 deletions(-)
diff --git a/block/iscsi.c b/block/iscsi.c
index d0b1a10..f12148e 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -199,8 +199,10 @@ iscsi_aio_write16_cb(struct iscsi_context *iscsi, int
status,
trace_iscsi_aio_write16_cb(iscsi, status, acb, acb->canceled);
-g_free(acb->buf);
-
+if (acb->buf != NULL) {
+g_free(acb->buf);
+}
+
if (acb->canceled != 0) {
return;
}
@@ -244,11 +246,18 @@ iscsi_aio_writev(BlockDriverState *bs, int64_t sector_num,
acb->bh = NULL;
acb->status = -EINPROGRESS;
-/* XXX we should pass the iovec to write16 to avoid the extra copy */
-/* this will allow us to get rid of 'buf' completely */
size = nb_sectors * BDRV_SECTOR_SIZE;
-acb->buf = g_malloc(size);
-qemu_iovec_to_buf(acb->qiov, 0, acb->buf, size);
+data.size = size;
+
+/* if the iovec only contains one buffer we can pass it directly */
+if (acb->qiov->niov == 1) {
+acb->buf = NULL;
+data.data = acb->qiov->iov[0].iov_base;
+} else {
+acb->buf = g_malloc(size);
+qemu_iovec_to_buf(acb->qiov, 0, acb->buf, size);
+data.data = acb->buf;
+}
acb->task = malloc(sizeof(struct scsi_task));
if (acb->task == NULL) {
@@ -269,9 +278,6 @@ iscsi_aio_writev(BlockDriverState *bs, int64_t sector_num,
*(uint32_t *)&acb->task->cdb[10] = htonl(num_sectors);
acb->task->expxferlen = size;
-data.data = acb->buf;
-data.size = size;
-
if (iscsi_scsi_command_async(iscsi, iscsilun->lun, acb->task,
iscsi_aio_write16_cb,
&data,
--
1.7.9.5
Re: [Qemu-devel] scsi-hd with discard_granularity and unmap results in Aborted Commands
Am 19.11.2012 15:41, schrieb Paolo Bonzini:
Il 19/11/2012 15:28, Stefan Priebe - Profihost AG ha scritto:
typedef struct RADOSCB {
@@ -376,6 +377,10 @@ static void qemu_rbd_complete_aio(RADOSCB *rcb)
RBDAIOCB *acb = rcb->acb;
int64_t r;
+if (acb->bh) {
+return;
+}
+
r = rcb->ret;
if (acb->cmd == RBD_AIO_WRITE ||
@@ -560,6 +565,20 @@ static void qemu_rbd_close(BlockDriverState *bs)
rados_shutdown(s->cluster);
}
+static void qemu_rbd_aio_abort(void *private_data)
+{
+RBDAIOCB *acb = (RBDAIOCB *) private_data;
+
+acb->status = -ECANCELED;
+
+if (acb->bh) {
+return;
+}
+
+acb->bh = qemu_bh_new(rbd_aio_bh_cb, acb);
+qemu_bh_schedule(acb->bh);
+}
I think this is all unneeded. Just store rcb->ret into
rcb->acb->status, and your version of qemu_rbd_aio_cancel should just work.
Also, I think the acb->cancelled field is not necessary anymore after
these changes.
The iscsi driver still relies on canceled that's why i left it here in too.
So you mean in qemu_rbd_complete_aio i should remove the check for
cancelled and then just overwrite acb->status to that it changes from
-EINPROGRESS to something else?
And qemu_rbd_aio_cancel should just wait for status != -EINPROGRESS?
Stefan
Re: [Qemu-devel] scsi-hd with discard_granularity and unmap results in Aborted Commands
Il 19/11/2012 15:28, Stefan Priebe - Profihost AG ha scritto:
> typedef struct RADOSCB {
> @@ -376,6 +377,10 @@ static void qemu_rbd_complete_aio(RADOSCB *rcb)
> RBDAIOCB *acb = rcb->acb;
> int64_t r;
>
> +if (acb->bh) {
> +return;
> +}
> +
> r = rcb->ret;
>
> if (acb->cmd == RBD_AIO_WRITE ||
> @@ -560,6 +565,20 @@ static void qemu_rbd_close(BlockDriverState *bs)
> rados_shutdown(s->cluster);
> }
>
> +static void qemu_rbd_aio_abort(void *private_data)
> +{
> +RBDAIOCB *acb = (RBDAIOCB *) private_data;
> +
> +acb->status = -ECANCELED;
> +
> +if (acb->bh) {
> +return;
> +}
> +
> +acb->bh = qemu_bh_new(rbd_aio_bh_cb, acb);
> +qemu_bh_schedule(acb->bh);
> +}
I think this is all unneeded. Just store rcb->ret into
rcb->acb->status, and your version of qemu_rbd_aio_cancel should just work.
Also, I think the acb->cancelled field is not necessary anymore after
these changes.
Paolo
Re: [Qemu-devel] scsi-hd with discard_granularity and unmap results in Aborted Commands
Il 19/11/2012 15:16, Stefan Priebe - Profihost AG ha scritto: > Hi Paolo, > Hi Josh, > > i started to port the iscsi fixes to rbd. The one think i'm missing is. > How to tell / call rbd to cancel I/O on the server side? > > I grepped librbd source code for abort / cancel but didn't find anything. > > Qemu must be able to tell rbd to cancel a specific I/O request. Just don't. QEMU will wait for rbd to finish the operation, instead of actually having it cancelled. Paolo
Re: [Qemu-devel] scsi-hd with discard_granularity and unmap results in Aborted Commands
Hi Paolo,
this is my current work status on porting these fixes to rbd. Right now
the discards get still canceled by the client kernel.
Might you have a look what i have forgotten?
Thanks!
Stefan
Am 19.11.2012 14:06, schrieb Paolo Bonzini:
Il 19/11/2012 14:01, Stefan Priebe - Profihost AG ha scritto:
The right behavior is to return
only after the target says whether the cancellation was done or not.
For libiscsi, it was implemented by the commits you mention.
So the whole bunch of changes is needed for rbd?
Something like the first three:
1bd075f29ea6d11853475c7c42734595720c3ac6
cfb3f5064af2d2e29c976e292c9472dfe9d61e31
27cbd828c617944c0f9603763fdf4fa87e7ad923
Paolo
>From 486fdb8b18310ff32ca64fbb2e0217c37319cff4 Mon Sep 17 00:00:00 2001
From: Stefan Priebe
Date: Mon, 19 Nov 2012 14:31:40 +0100
Subject: [PATCH 1/2] do not check for cancellation in qemu_rbd_complete_aio
Signed-off-by: Stefan Priebe
---
block/rbd.c |8 +---
1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/block/rbd.c b/block/rbd.c
index 5a0f79f..583bcc3 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -376,12 +376,6 @@ static void qemu_rbd_complete_aio(RADOSCB *rcb)
RBDAIOCB *acb = rcb->acb;
int64_t r;
-if (acb->cancelled) {
-qemu_vfree(acb->bounce);
-qemu_aio_release(acb);
-goto done;
-}
-
r = rcb->ret;
if (acb->cmd == RBD_AIO_WRITE ||
@@ -409,7 +403,7 @@ static void qemu_rbd_complete_aio(RADOSCB *rcb)
/* Note that acb->bh can be NULL in case where the aio was cancelled */
acb->bh = qemu_bh_new(rbd_aio_bh_cb, acb);
qemu_bh_schedule(acb->bh);
-done:
+
g_free(rcb);
}
--
1.7.10.4
>From e9eac2c7ed7b98ff102ab7da4573f081ebca32fa Mon Sep 17 00:00:00 2001
From: Stefan Priebe
Date: Mon, 19 Nov 2012 15:01:16 +0100
Subject: [PATCH 2/2] rbd: fix races between io completition and abort
Signed-off-by: Stefan Priebe
---
block/rbd.c | 40 +++-
1 file changed, 39 insertions(+), 1 deletion(-)
diff --git a/block/rbd.c b/block/rbd.c
index 583bcc3..ae1d03b 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -77,6 +77,7 @@ typedef struct RBDAIOCB {
int error;
struct BDRVRBDState *s;
int cancelled;
+int status;
} RBDAIOCB;
typedef struct RADOSCB {
@@ -376,6 +377,10 @@ static void qemu_rbd_complete_aio(RADOSCB *rcb)
RBDAIOCB *acb = rcb->acb;
int64_t r;
+if (acb->bh) {
+return;
+}
+
r = rcb->ret;
if (acb->cmd == RBD_AIO_WRITE ||
@@ -560,6 +565,20 @@ static void qemu_rbd_close(BlockDriverState *bs)
rados_shutdown(s->cluster);
}
+static void qemu_rbd_aio_abort(void *private_data)
+{
+RBDAIOCB *acb = (RBDAIOCB *) private_data;
+
+acb->status = -ECANCELED;
+
+if (acb->bh) {
+return;
+}
+
+acb->bh = qemu_bh_new(rbd_aio_bh_cb, acb);
+qemu_bh_schedule(acb->bh);
+}
+
/*
* Cancel aio. Since we don't reference acb in a non qemu threads,
* it is safe to access it here.
@@ -567,7 +586,22 @@ static void qemu_rbd_close(BlockDriverState *bs)
static void qemu_rbd_aio_cancel(BlockDriverAIOCB *blockacb)
{
RBDAIOCB *acb = (RBDAIOCB *) blockacb;
+
+if (acb->status != -EINPROGRESS) {
+return;
+}
+
acb->cancelled = 1;
+
+// TODO / FIXME: send an abort command to rbd
+// Normally we should call abort librbd and
+// librbd gets qemu_rbd_aio_abort as a callback function
+// i wasn't able to find an abort function in librbd at all
+qemu_rbd_aio_abort(acb);
+
+while (acb->status == -EINPROGRESS) {
+qemu_aio_wait();
+}
}
static AIOPool rbd_aio_pool = {
@@ -636,10 +670,13 @@ static void rbd_aio_bh_cb(void *opaque)
qemu_iovec_from_buf(acb->qiov, 0, acb->bounce, acb->qiov->size);
}
qemu_vfree(acb->bounce);
-acb->common.cb(acb->common.opaque, (acb->ret > 0 ? 0 : acb->ret));
qemu_bh_delete(acb->bh);
acb->bh = NULL;
+if (acb->cancelled == 0) {
+acb->common.cb(acb->common.opaque, (acb->ret > 0 ? 0 : acb->ret));
+}
+
qemu_aio_release(acb);
}
@@ -685,6 +722,7 @@ static BlockDriverAIOCB *rbd_start_aio(BlockDriverState *bs,
acb->s = s;
acb->cancelled = 0;
acb->bh = NULL;
+acb->status = -EINPROGRESS;
if (cmd == RBD_AIO_WRITE) {
qemu_iovec_to_buf(acb->qiov, 0, acb->bounce, qiov->size);
--
1.7.10.4
[Qemu-devel] [PATCH 1/2] i2c: Introduce device address mask
Some devices react on multiple addresses. To emulate this, we could
register them multiple times, but that is cumbersome. The AT24C16, e.g.
listens on 8 different addresses.
Instead, introduce a device address mask that is applied on the
transmitted address before matching it against the stored one. Moreover,
the transmitted address is passed as additional parameter to the event
callback of the device.
Signed-off-by: Jan Kiszka
---
hw/ds1338.c |2 +-
hw/i2c.c |9 +
hw/i2c.h |3 ++-
hw/lm832x.c |2 +-
hw/max7310.c |2 +-
hw/pxa2xx.c |3 ++-
hw/smbus.c|2 +-
hw/ssd0303.c |2 +-
hw/tmp105.c |2 +-
hw/tosa.c |2 +-
hw/twl92230.c |2 +-
hw/wm8750.c |2 +-
hw/z2.c |2 +-
13 files changed, 19 insertions(+), 16 deletions(-)
diff --git a/hw/ds1338.c b/hw/ds1338.c
index b576d56..59fcc01 100644
--- a/hw/ds1338.c
+++ b/hw/ds1338.c
@@ -75,7 +75,7 @@ static void inc_regptr(DS1338State *s)
}
}
-static void ds1338_event(I2CSlave *i2c, enum i2c_event event)
+static void ds1338_event(I2CSlave *i2c, enum i2c_event event, uint8_t param)
{
DS1338State *s = FROM_I2C_SLAVE(DS1338State, i2c);
diff --git a/hw/i2c.c b/hw/i2c.c
index 296bece..72b8f07 100644
--- a/hw/i2c.c
+++ b/hw/i2c.c
@@ -93,7 +93,7 @@ int i2c_start_transfer(i2c_bus *bus, uint8_t address, int
recv)
QTAILQ_FOREACH(kid, &bus->qbus.children, sibling) {
DeviceState *qdev = kid->child;
I2CSlave *candidate = I2C_SLAVE_FROM_QDEV(qdev);
-if (candidate->address == address) {
+if (candidate->address == (address & candidate->address_mask)) {
slave = candidate;
break;
}
@@ -108,7 +108,7 @@ int i2c_start_transfer(i2c_bus *bus, uint8_t address, int
recv)
start condition. */
bus->current_dev = slave;
if (sc->event) {
-sc->event(slave, recv ? I2C_START_RECV : I2C_START_SEND);
+sc->event(slave, recv ? I2C_START_RECV : I2C_START_SEND, address);
}
return 0;
}
@@ -124,7 +124,7 @@ void i2c_end_transfer(i2c_bus *bus)
sc = I2C_SLAVE_GET_CLASS(dev);
if (sc->event) {
-sc->event(dev, I2C_FINISH);
+sc->event(dev, I2C_FINISH, 0);
}
bus->current_dev = NULL;
@@ -175,7 +175,7 @@ void i2c_nack(i2c_bus *bus)
sc = I2C_SLAVE_GET_CLASS(dev);
if (sc->event) {
-sc->event(dev, I2C_NACK);
+sc->event(dev, I2C_NACK, 0);
}
}
@@ -207,6 +207,7 @@ static int i2c_slave_qdev_init(DeviceState *dev)
I2CSlave *s = I2C_SLAVE_FROM_QDEV(dev);
I2CSlaveClass *sc = I2C_SLAVE_GET_CLASS(s);
+s->address_mask = 0x7f;
return sc->init(s);
}
diff --git a/hw/i2c.h b/hw/i2c.h
index 0f5682b..6cf164b 100644
--- a/hw/i2c.h
+++ b/hw/i2c.h
@@ -39,7 +39,7 @@ typedef struct I2CSlaveClass
int (*recv)(I2CSlave *s);
/* Notify the slave of a bus state change. */
-void (*event)(I2CSlave *s, enum i2c_event event);
+void (*event)(I2CSlave *s, enum i2c_event event, uint8_t param);
} I2CSlaveClass;
struct I2CSlave
@@ -48,6 +48,7 @@ struct I2CSlave
/* Remaining fields for internal use by the I2C code. */
uint8_t address;
+uint8_t address_mask;
};
i2c_bus *i2c_init_bus(DeviceState *parent, const char *name);
diff --git a/hw/lm832x.c b/hw/lm832x.c
index 8e09f9b..d69aa4b 100644
--- a/hw/lm832x.c
+++ b/hw/lm832x.c
@@ -378,7 +378,7 @@ static void lm_kbd_write(LM823KbdState *s, int reg, int
byte, uint8_t value)
}
}
-static void lm_i2c_event(I2CSlave *i2c, enum i2c_event event)
+static void lm_i2c_event(I2CSlave *i2c, enum i2c_event event, uint8_t param)
{
LM823KbdState *s = FROM_I2C_SLAVE(LM823KbdState, i2c);
diff --git a/hw/max7310.c b/hw/max7310.c
index 1ed18ba..63999be 100644
--- a/hw/max7310.c
+++ b/hw/max7310.c
@@ -123,7 +123,7 @@ static int max7310_tx(I2CSlave *i2c, uint8_t data)
return 0;
}
-static void max7310_event(I2CSlave *i2c, enum i2c_event event)
+static void max7310_event(I2CSlave *i2c, enum i2c_event event, uint8_t param)
{
MAX7310State *s = (MAX7310State *) i2c;
s->len = 0;
diff --git a/hw/pxa2xx.c b/hw/pxa2xx.c
index e616979..fb1fd0d 100644
--- a/hw/pxa2xx.c
+++ b/hw/pxa2xx.c
@@ -1239,7 +1239,8 @@ static void pxa2xx_i2c_update(PXA2xxI2CState *s)
}
/* These are only stubs now. */
-static void pxa2xx_i2c_event(I2CSlave *i2c, enum i2c_event event)
+static void pxa2xx_i2c_event(I2CSlave *i2c, enum i2c_event event,
+ uint8_t param)
{
PXA2xxI2CSlaveState *slave = FROM_I2C_SLAVE(PXA2xxI2CSlaveState, i2c);
PXA2xxI2CState *s = slave->host;
diff --git a/hw/smbus.c b/hw/smbus.c
index e3cf6a2..7d070d9 100644
--- a/hw/smbus.c
+++ b/hw/smbus.c
@@ -66,7 +66,7 @@ static void smbus_do_write(SMBusDevice *dev)
}
}
-static void smbus_i2c_event(I2CSlave *s, enum i2c_event event)
+static void smbus_i2c_event(I2CSlave *s, enum i2c_event event, uint8_t param)
{
SMBusDevice *dev = SMBUS_D
[Qemu-devel] [PATCH 2/2] Add AT24Cxx I2C EEPROM device model
This implements I2C EEPROMs of the AT24Cxx series. Sizes from 1Kbit to
1024Kbit are supported. Each EEPROM is backed by a block device. Its
size can be explicitly specified by the "size" property (required for
sizes < 512, the blockdev sector size) or is derived from the size of
the backing block device. Device addresses are built from the device
number property. Write protection can be configured by declaring the
block device read-only.
Signed-off-by: Jan Kiszka
---
hw/Makefile.objs |2 +-
hw/at24.c| 363 ++
2 files changed, 364 insertions(+), 1 deletions(-)
create mode 100644 hw/at24.c
diff --git a/hw/Makefile.objs b/hw/Makefile.objs
index ea46f81..0c64712 100644
--- a/hw/Makefile.objs
+++ b/hw/Makefile.objs
@@ -170,7 +170,7 @@ common-obj-$(CONFIG_SSD0323) += ssd0323.o
common-obj-$(CONFIG_ADS7846) += ads7846.o
common-obj-$(CONFIG_MAX111X) += max111x.o
common-obj-$(CONFIG_DS1338) += ds1338.o
-common-obj-y += i2c.o smbus.o smbus_eeprom.o
+common-obj-y += i2c.o smbus.o smbus_eeprom.o at24.o
common-obj-y += eeprom93xx.o
common-obj-y += scsi-disk.o cdrom.o hd-geometry.o block-common.o
common-obj-y += scsi-generic.o scsi-bus.o
diff --git a/hw/at24.c b/hw/at24.c
new file mode 100644
index 000..e34f2da
--- /dev/null
+++ b/hw/at24.c
@@ -0,0 +1,363 @@
+/*
+ * AT24Cxx EEPROM emulation
+ *
+ * Copyright (c) 2012 Siemens AG
+ * Author: Jan Kiszka
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "hw.h"
+#include "i2c.h"
+#include "blockdev.h"
+#include "hw/block-common.h"
+
+#define AT24_BASE_ADDRESS 0x50
+#define AT24_MAX_PAGE_LEN 256
+
+typedef enum AT24TransferState {
+AT24_IDLE,
+AT24_RD_ADDR,
+AT24_WR_ADDR_HI,
+AT24_WR_ADDR_LO,
+AT24_RW_DATA0,
+AT24_RD_DATA,
+AT24_WR_DATA,
+} AT24TransferState;
+
+typedef struct AT24State {
+I2CSlave i2c;
+BlockConf block_conf;
+BlockDriverState *bs;
+uint32_t size;
+bool wp;
+unsigned int addr_mask;
+unsigned int page_mask;
+bool addr16;
+unsigned int hi_addr_mask;
+uint8_t device;
+AT24TransferState transfer_state;
+uint8_t sector_buffer[BDRV_SECTOR_SIZE];
+int cached_sector;
+bool cache_dirty;
+uint32_t transfer_addr;
+} AT24State;
+
+static void at24_flush_transfer_buffer(AT24State *s)
+{
+if (s->cached_sector < 0 || !s->cache_dirty) {
+return;
+}
+bdrv_write(s->bs, s->cached_sector, s->sector_buffer, 1);
+s->cache_dirty = false;
+}
+
+static void at24_event(I2CSlave *i2c, enum i2c_event event, uint8_t param)
+{
+AT24State *s = DO_UPCAST(AT24State, i2c, i2c);
+
+switch (event) {
+case I2C_START_SEND:
+switch (s->transfer_state) {
+case AT24_IDLE:
+if (s->addr16) {
+s->transfer_addr = (param & s->hi_addr_mask) << 16;
+s->transfer_state = AT24_WR_ADDR_HI;
+} else {
+s->transfer_addr = (param & s->hi_addr_mask) << 8;
+s->transfer_state = AT24_WR_ADDR_LO;
+}
+break;
+default:
+s->transfer_state = AT24_IDLE;
+break;
+}
+break;
+case I2C_START_RECV:
+switch (s->transfer_state) {
+case AT24_IDLE:
+s->transfer_state = AT24_RD_ADDR;
+break;
+case AT24_RW_DATA0:
+s->transfer_state = AT24_RD_DATA;
+break;
+default:
+s->transfer_state = AT24_IDLE;
+break;
+}
+break;
+case I2C_FINISH:
+switch (s->transfer_state) {
+case AT24_WR_DATA:
+at24_flush_transfer_buffer(s);
+/* fall through */
+default:
+s->transfer_state = AT24_IDLE;
+break;
+}
+break;
+default:
+s->transfer_state = AT24_IDLE;
+break;
+}
+}
+
+static int at24_cache_sector(AT2
[Qemu-devel] [PATCH 0/2] i2c: Add AT24Cxx EEPROM model
See patches for details. Jan Kiszka (2): i2c: Introduce device address mask Add AT24Cxx I2C EEPROM device model hw/Makefile.objs |2 +- hw/at24.c| 363 ++ hw/ds1338.c |2 +- hw/i2c.c |9 +- hw/i2c.h |3 +- hw/lm832x.c |2 +- hw/max7310.c |2 +- hw/pxa2xx.c |3 +- hw/smbus.c |2 +- hw/ssd0303.c |2 +- hw/tmp105.c |2 +- hw/tosa.c|2 +- hw/twl92230.c|2 +- hw/wm8750.c |2 +- hw/z2.c |2 +- 15 files changed, 383 insertions(+), 17 deletions(-) create mode 100644 hw/at24.c -- 1.7.3.4
Re: [Qemu-devel] scsi-hd with discard_granularity and unmap results in Aborted Commands
Hi Paolo, Hi Josh, i started to port the iscsi fixes to rbd. The one think i'm missing is. How to tell / call rbd to cancel I/O on the server side? I grepped librbd source code for abort / cancel but didn't find anything. Qemu must be able to tell rbd to cancel a specific I/O request. Greets, Stefan Am 19.11.2012 14:06, schrieb Paolo Bonzini: Il 19/11/2012 14:01, Stefan Priebe - Profihost AG ha scritto: The right behavior is to return only after the target says whether the cancellation was done or not. For libiscsi, it was implemented by the commits you mention. So the whole bunch of changes is needed for rbd? Something like the first three: 1bd075f29ea6d11853475c7c42734595720c3ac6 cfb3f5064af2d2e29c976e292c9472dfe9d61e31 27cbd828c617944c0f9603763fdf4fa87e7ad923 Paolo
[Qemu-devel] [PATCH 0/2] [PULL] slirp: Fix for ARP assert and DSN search DHCP option
The following changes since commit ce34cf72fe508b27a78f83c184142e8d1e6a048a: Merge remote-tracking branch 'awilliam/tags/vfio-pci-for-qemu-1.3.0-rc0' into staging (2012-11-14 08:53:40 -0600) are available in the git repository at: git://git.kiszka.org/qemu.git queues/slirp Klaus Stengel (1): slirp: Add domain-search option to slirp's DHCP server Nickolai Zeldovich (1): slirp: Don't crash on packets from 0.0.0.0/8. net/slirp.c | 35 +- qapi-schema.json|4 + qemu-options.hx | 18 +++- slirp/Makefile.objs |2 +- slirp/arp_table.c |4 +- slirp/bootp.c | 12 ++ slirp/dnssearch.c | 314 +++ slirp/libslirp.h|3 +- slirp/slirp.c |8 +- slirp/slirp.h |5 + 10 files changed, 395 insertions(+), 10 deletions(-) create mode 100644 slirp/dnssearch.c CC: Klaus Stengel CC: Nickolai Zeldovich Klaus Stengel (1): slirp: Add domain-search option to slirp's DHCP server Nickolai Zeldovich (1): slirp: Don't crash on packets from 0.0.0.0/8. net/slirp.c | 35 +- qapi-schema.json|4 + qemu-options.hx | 18 +++- slirp/Makefile.objs |2 +- slirp/arp_table.c |4 +- slirp/bootp.c | 12 ++ slirp/dnssearch.c | 314 +++ slirp/libslirp.h|3 +- slirp/slirp.c |8 +- slirp/slirp.h |5 + 10 files changed, 395 insertions(+), 10 deletions(-) create mode 100644 slirp/dnssearch.c -- 1.7.3.4
[Qemu-devel] [PATCH 2/2] slirp: Add domain-search option to slirp's DHCP server
From: Klaus Stengel
This patch will allow the user to include the domain-search option in
replies from the built-in DHCP server. The domain suffixes can be
specified by adding dnssearch= entries to the "-net user" parameter.
[Jan: tiny style adjustments]
Signed-off-by: Klaus Stengel
Signed-off-by: Jan Kiszka
---
net/slirp.c | 35 +-
qapi-schema.json|4 +
qemu-options.hx | 18 +++-
slirp/Makefile.objs |2 +-
slirp/bootp.c | 12 ++
slirp/dnssearch.c | 314 +++
slirp/libslirp.h|3 +-
slirp/slirp.c |8 +-
slirp/slirp.h |5 +
9 files changed, 392 insertions(+), 9 deletions(-)
create mode 100644 slirp/dnssearch.c
diff --git a/net/slirp.c b/net/slirp.c
index bf86a44..afb52c3 100644
--- a/net/slirp.c
+++ b/net/slirp.c
@@ -136,7 +136,7 @@ static int net_slirp_init(NetClientState *peer, const char
*model,
const char *vhostname, const char *tftp_export,
const char *bootfile, const char *vdhcp_start,
const char *vnameserver, const char *smb_export,
- const char *vsmbserver)
+ const char *vsmbserver, const char **dnssearch)
{
/* default settings according to historic slirp */
struct in_addr net = { .s_addr = htonl(0x0a000200) }; /* 10.0.2.0 */
@@ -242,7 +242,7 @@ static int net_slirp_init(NetClientState *peer, const char
*model,
s = DO_UPCAST(SlirpState, nc, nc);
s->slirp = slirp_init(restricted, net, mask, host, vhostname,
- tftp_export, bootfile, dhcp, dns, s);
+ tftp_export, bootfile, dhcp, dns, dnssearch, s);
QTAILQ_INSERT_TAIL(&slirp_stacks, s, entry);
for (config = slirp_configs; config; config = config->next) {
@@ -699,6 +699,31 @@ net_init_slirp_configs(const StringList *fwd, int flags)
}
}
+static const char **slirp_dnssearch(const StringList *dnsname)
+{
+const StringList *c = dnsname;
+size_t i = 0, num_opts = 0;
+const char **ret;
+
+while (c) {
+num_opts++;
+c = c->next;
+}
+
+if (num_opts == 0) {
+return NULL;
+}
+
+ret = g_malloc((num_opts + 1) * sizeof(*ret));
+c = dnsname;
+while (c) {
+ret[i++] = c->value->str;
+c = c->next;
+}
+ret[i] = NULL;
+return ret;
+}
+
int net_init_slirp(const NetClientOptions *opts, const char *name,
NetClientState *peer)
{
@@ -706,6 +731,7 @@ int net_init_slirp(const NetClientOptions *opts, const char
*name,
char *vnet;
int ret;
const NetdevUserOptions *user;
+const char **dnssearch;
assert(opts->kind == NET_CLIENT_OPTIONS_KIND_USER);
user = opts->user;
@@ -714,6 +740,8 @@ int net_init_slirp(const NetClientOptions *opts, const char
*name,
user->has_ip ? g_strdup_printf("%s/24", user->ip) :
NULL;
+dnssearch = slirp_dnssearch(user->dnssearch);
+
/* all optional fields are initialized to "all bits zero" */
net_init_slirp_configs(user->hostfwd, SLIRP_CFG_HOSTFWD);
@@ -722,7 +750,7 @@ int net_init_slirp(const NetClientOptions *opts, const char
*name,
ret = net_slirp_init(peer, "user", name, user->q_restrict, vnet,
user->host, user->hostname, user->tftp,
user->bootfile, user->dhcpstart, user->dns, user->smb,
- user->smbserver);
+ user->smbserver, dnssearch);
while (slirp_configs) {
config = slirp_configs;
@@ -731,6 +759,7 @@ int net_init_slirp(const NetClientOptions *opts, const char
*name,
}
g_free(vnet);
+g_free(dnssearch);
return ret;
}
diff --git a/qapi-schema.json b/qapi-schema.json
index 542e3ac..5dfa052 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2404,6 +2404,9 @@
#
# @dns: #optional guest-visible address of the virtual nameserver
#
+# @dnssearch: #optional list of DNS suffixes to search, passed as DHCP option
+# to the guest
+#
# @smb: #optional root directory of the built-in SMB server
#
# @smbserver: #optional IP address of the built-in SMB server
@@ -2426,6 +2429,7 @@
'*bootfile': 'str',
'*dhcpstart': 'str',
'*dns': 'str',
+'*dnssearch': ['String'],
'*smb': 'str',
'*smbserver': 'str',
'*hostfwd': ['String'],
diff --git a/qemu-options.hx b/qemu-options.hx
index fe8f15c..a165cff 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1318,8 +1318,8 @@ DEF("net", HAS_ARG, QEMU_OPTION_net,
"create a new Network Interface Card and connect it to
VLAN 'n'\n"
#ifdef CONFIG_SLIRP
"-net
user[,vlan=n][,name=str][,net=addr[/mask]][,host=addr][,restrict=on|off]\n"
-"
[,hostname=host][,dhcpstart=addr][,dns=addr][,tftp=dir][,bootfile=f]\n"
-" [,hostfwd=rule][,guestfwd=rule]"
[Qemu-devel] [PATCH 1/2] slirp: Don't crash on packets from 0.0.0.0/8.
From: Nickolai Zeldovich
LWIP can generate packets with a source of 0.0.0.0, which triggers an
assertion failure in arp_table_add(). Instead of crashing, simply return
to avoid adding an invalid ARP table entry.
Signed-off-by: Nickolai Zeldovich
Signed-off-by: Jan Kiszka
---
slirp/arp_table.c |4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/slirp/arp_table.c b/slirp/arp_table.c
index 5d7b8ac..bf698c1 100644
--- a/slirp/arp_table.c
+++ b/slirp/arp_table.c
@@ -38,7 +38,9 @@ void arp_table_add(Slirp *slirp, uint32_t ip_addr, uint8_t
ethaddr[ETH_ALEN])
ethaddr[3], ethaddr[4], ethaddr[5]));
/* Check 0.0.0.0/8 invalid source-only addresses */
-assert((ip_addr & htonl(~(0xf << 28))) != 0);
+if ((ip_addr & htonl(~(0xf << 28))) == 0) {
+return;
+}
if (ip_addr == 0x || ip_addr == broadcast_addr) {
/* Do not register broadcast addresses */
--
1.7.3.4
Re: [Qemu-devel] [Bug 1080086] Re: MC146818 RTC breaks when SET bit in Register B is on.
>> Out of curiosity, does anyone know how long this particular bug has >> been undetected or how/when it was introduced? > > Probably it was introduced last September when the model was rewritten. > But it's really unlikely that the bug would have been detected. Thanks for that quick assessment. > On the other hand, the bug in commit b6db4ac (which also includes a > qtest) was detected by autotest. Could your tool find it, too? That's a very respectable achievement. I understand that Autotest runs predefined (i.e. hand-written) tests on a large scale. In contrast, we seek a higher degree of automation but on a smaller scale. These differences could make a comparison difficult. However, your example is much appreciated because it helps us to set our work in perspective. >> This could help me explain to others my research interest in symbolic >> execution of hardware models and its application in form of automated >> test generation. > > Very interesting (at least to me :)). Perhaps you find the following background helpful. To kick start our work, we extracted from QEMU a standalone RTC hardware model [2] because QOM, QMP, TCG or QTest would render the semi-automatic analysis infeasible. As a next step, we would like to combine this standalone hardware model with a Linux x86 driver [3]. The reported bug is a good exemplar of the type of firmware/hardware interface properties we are interested in. Note that this is only the initial phase of our research and there is much work yet to be done. Of course, any comments or collaboration are always welcome. With kind regards, Alex [2] https://github.com/ahorn/benchmarks/tree/master/qemu-hw [3] https://github.com/ahorn/benchmarks/tree/master/sw-hw/linux/rtc_x86 On 19 November 2012 11:42, Paolo Bonzini wrote: > Il 19/11/2012 12:34, Alex Horn ha scritto: >>> [...] the patch is almost good for inclusion. I'd ask for two changes: >>> 1) please test == 0, not != REG_B_SET; >>> 2) please leave the fuzzicsng test last >> >> I have attached a new patch with the requested changes. >> >> This patch also improves the quality of the functional test by >> checking that RTC_SECONDS is equal (==) to the previously written data >> provided the SET flag in Register B is still enabled. This is >> justified by the data sheet which states that an enabled SET bit >> "stops an existing update" and prevents "a new one from occurring" [1, >> p. 15]. In contrast, once the SET flag is disabled, the RTC_SECONDS >> check uses an inequality (>=) as in the original test case. > > Right. > >> Out of curiosity, does anyone know how long this particular bug has >> been undetected or how/when it was introduced? > > Probably it was introduced last September when the model was rewritten. > But it's really unlikely that the bug would have been detected. > > On the other hand, the bug in commit b6db4ac (which also includes a > qtest) was detected by autotest. Could your tool find it, too? > >> This could help me explain to others my research interest in symbolic >> execution of hardware models and its application in form of automated >> test generation. > > Very interesting (at least to me :)). > >> Finally, if there is interest to improve the robustness of the RTC >> model, I could send a patch with several verification conditions (i.e. >> assertions) which can help to expose these kind of bugs in the RTC >> hardware model. > > Sure, that's welcome. > > In particular, I assume you verified the "next alarm" code to be correct? :) > > Paolo > >> Recall that most compiler can usually optimize these >> assertions away unless a developer explicitly enables them. They also >> serve as unambiguous code documentation. >> >> With best regards, >> Alex >> >> [1] >> http://www.freescale.com/files/microcontrollers/doc/data_sheet/MC146818.pdf >> >> On 18 November 2012 08:52, Paolo Bonzini wrote: >>> Il 17/11/2012 19:47, Alex Horn ha scritto: I have attached a patch for the most recent version of the file hw/mc146818rtc.c [1]. The patch also features a functional test which executes through the QTest framework. I would appreciate your thoughts on this. [1] http://git.qemu.org/?p=qemu.git;a=blob;f=hw/mc146818rtc.c;h=98839f278d93452d071054e2a017b3d909b45ab2;hb=9cb535fe4ef08b01e583ec955767a0899ff79afe#l563 ** Patch added: "register_b_set_flag.patch" https://bugs.launchpad.net/qemu/+bug/1080086/+attachment/3436808/+files/register_b_set_flag.patch >>> >>> Hi Alex, the patch is almost good for inclusion. I'd ask for two >>> changes: 1) please test == 0, not != REG_B_SET; 2) please leave the >>> fuzzing test last, because it may leave some registers in an undefined >>> state. >>> >>> Paolo >
Re: [Qemu-devel] [PATCH 1/1] Support default block interfaces per QEMUMachine
Alexander Graf writes:
> On 19.11.2012, at 14:44, Christian Borntraeger wrote:
>
>> On 19/11/12 14:36, Alexander Graf wrote:
>>>
>>> On 12.11.2012, at 09:22, Christian Borntraeger wrote:
>>>
There are QEMUMachines that have neither IF_IDE nor IF_SCSI as a
default/standard interface to their block devices / drives. Therfore,
this patch introduces a new field default_block per QEMUMachine struct.
The prior use_scsi field becomes thereby obsolete and is replaced through
.default_block = DEFAULT_SCSI.
Based on an initial patch from Einar Lueck
Signed-off-by: Christian Borntraeger
---
blockdev.c |4 ++--
blockdev.h | 29 -
hw/boards.h |3 ++-
hw/device-hotplug.c |2 +-
hw/highbank.c |2 +-
hw/leon3.c |1 -
hw/mips_jazz.c |4 ++--
hw/pc_sysfw.c |2 +-
hw/puv3.c |1 -
hw/realview.c |7 ---
hw/s390-virtio.c| 16 +---
hw/spapr.c |2 +-
hw/sun4m.c | 24
hw/versatilepb.c|4 ++--
hw/vexpress.c |4 ++--
hw/xilinx_zynq.c|2 +-
vl.c| 20 +++-
17 files changed, 71 insertions(+), 56 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index e73fd6e..aca3c14 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -275,7 +275,7 @@ static bool do_check_io_limits(BlockIOLimit *io_limits)
return true;
}
-DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
+DriveInfo *drive_init(QemuOpts *opts, BlockDefault block_default)
{
const char *buf;
const char *file = NULL;
@@ -325,7 +325,7 @@ DriveInfo *drive_init(QemuOpts *opts, int
default_to_scsi)
return NULL;
}
} else {
-type = default_to_scsi ? IF_SCSI : IF_IDE;
+type = block_if_from_default(block_default);
}
max_devs = if_max_devs[type];
diff --git a/blockdev.h b/blockdev.h
index 5f27b64..aba6d77 100644
--- a/blockdev.h
+++ b/blockdev.h
@@ -24,6 +24,33 @@ typedef enum {
IF_COUNT
} BlockInterfaceType;
+/* For machine default interface. */
+typedef enum {
+DEFAULT_IDE = 0,
+DEFAULT_SCSI,
+DEFAULT_FLOPPY,
+DEFAULT_PFLASH,
+DEFAULT_MTD,
+DEFAULT_SD,
+DEFAULT_VIRTIO,
+DEFAULT_XEN
+} BlockDefault;
>>>
>>> Why a new enum? Can't we just reuse the IF_ ones?
>>>
>>> Also, traditionally Markus has had very strong opinions on anything
>>> IF_ and -drive related. It's probably a good idea to get him in the
>>> loop :).
>>>
>>> Alex
>>
>> Review feedback from the first cycle. Anthony wanted to make the
>> default (field is not specified at all) a
>> sane default, which means IDE must be 0.
>
> IF_ are internal constants, we can change them to our liking. So if we
> just make IF_IDE=0, all is well, no? Worst case we can always have an
> IF_DEFAULT=0 that falls back to IF_IDE.
>
> I really dislike having the same list twice, just with different but
> almost identical names.
Generalizing QEMUMachine member use_scsi to a default BlockInterfaceType
makes plenty of sense to me.
I'm not sure "no initializer means IDE" is such a hot idea, but since
it's how use_scsi has always worked, I'm okay with making its
replacement work like that, too.
Like Alex, I dislike inventing yet another enumeration for this
purpose. Let's use BlockInterfaceType.
In theory, we can change values of internal enumerations freely. In
practice, such changes can run afoul of leaky abstractions, such as C89
array initializers and sloppy tests against zero. Careful review
advised.
Re: [Qemu-devel] slow virtio network with vhost=on and multiple cores
On 13.11.2012 17:33, Michael S. Tsirkin wrote: On Tue, Nov 13, 2012 at 06:22:56PM +0200, Michael S. Tsirkin wrote: On Tue, Nov 13, 2012 at 12:49:03PM +0100, Peter Lieven wrote: On 09.11.2012 19:03, Peter Lieven wrote: Remark: If i disable interrupts on CPU1-3 for virtio the performance is ok again. Now we need someone with deeper knowledge of the in-kernel irqchip and the virtio/vhost driver development to say if this is a regression in qemu-kvm or a problem with the old virtio drivers if they receive the interrupt on different CPUs. anyone? Looks like the problem is not in the guest: I tried ubuntu guest on a rhel host, I got 8GB/s with vhost and 4GB/s without on a host to guest banchmark. Tried with upstream qemu on rhel kernel and that's even a bit faster. So it's ubuntu kernel. vanilla 2.6.32 didn't have vhost at all so maybe their vhost backport is broken insome way. Do you remember since which version of the vanilla kernel vhost-net was officially included? Peter
Re: [Qemu-devel] [PATCH 1/1] Support default block interfaces per QEMUMachine
On 19.11.2012, at 14:44, Christian Borntraeger wrote:
> On 19/11/12 14:36, Alexander Graf wrote:
>>
>> On 12.11.2012, at 09:22, Christian Borntraeger wrote:
>>
>>> There are QEMUMachines that have neither IF_IDE nor IF_SCSI as a
>>> default/standard interface to their block devices / drives. Therfore,
>>> this patch introduces a new field default_block per QEMUMachine struct.
>>> The prior use_scsi field becomes thereby obsolete and is replaced through
>>> .default_block = DEFAULT_SCSI.
>>>
>>> Based on an initial patch from Einar Lueck
>>>
>>> Signed-off-by: Christian Borntraeger
>>> ---
>>> blockdev.c |4 ++--
>>> blockdev.h | 29 -
>>> hw/boards.h |3 ++-
>>> hw/device-hotplug.c |2 +-
>>> hw/highbank.c |2 +-
>>> hw/leon3.c |1 -
>>> hw/mips_jazz.c |4 ++--
>>> hw/pc_sysfw.c |2 +-
>>> hw/puv3.c |1 -
>>> hw/realview.c |7 ---
>>> hw/s390-virtio.c| 16 +---
>>> hw/spapr.c |2 +-
>>> hw/sun4m.c | 24
>>> hw/versatilepb.c|4 ++--
>>> hw/vexpress.c |4 ++--
>>> hw/xilinx_zynq.c|2 +-
>>> vl.c| 20 +++-
>>> 17 files changed, 71 insertions(+), 56 deletions(-)
>>>
>>> diff --git a/blockdev.c b/blockdev.c
>>> index e73fd6e..aca3c14 100644
>>> --- a/blockdev.c
>>> +++ b/blockdev.c
>>> @@ -275,7 +275,7 @@ static bool do_check_io_limits(BlockIOLimit *io_limits)
>>>return true;
>>> }
>>>
>>> -DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
>>> +DriveInfo *drive_init(QemuOpts *opts, BlockDefault block_default)
>>> {
>>>const char *buf;
>>>const char *file = NULL;
>>> @@ -325,7 +325,7 @@ DriveInfo *drive_init(QemuOpts *opts, int
>>> default_to_scsi)
>>>return NULL;
>>> }
>>>} else {
>>> -type = default_to_scsi ? IF_SCSI : IF_IDE;
>>> +type = block_if_from_default(block_default);
>>>}
>>>
>>>max_devs = if_max_devs[type];
>>> diff --git a/blockdev.h b/blockdev.h
>>> index 5f27b64..aba6d77 100644
>>> --- a/blockdev.h
>>> +++ b/blockdev.h
>>> @@ -24,6 +24,33 @@ typedef enum {
>>>IF_COUNT
>>> } BlockInterfaceType;
>>>
>>> +/* For machine default interface. */
>>> +typedef enum {
>>> +DEFAULT_IDE = 0,
>>> +DEFAULT_SCSI,
>>> +DEFAULT_FLOPPY,
>>> +DEFAULT_PFLASH,
>>> +DEFAULT_MTD,
>>> +DEFAULT_SD,
>>> +DEFAULT_VIRTIO,
>>> +DEFAULT_XEN
>>> +} BlockDefault;
>>
>> Why a new enum? Can't we just reuse the IF_ ones?
>>
>> Also, traditionally Markus has had very strong opinions on anything IF_ and
>> -drive related. It's probably a good idea to get him in the loop :).
>>
>> Alex
>
> Review feedback from the first cycle. Anthony wanted to make the default
> (field is not specified at all) a
> sane default, which means IDE must be 0.
IF_ are internal constants, we can change them to our liking. So if we just
make IF_IDE=0, all is well, no? Worst case we can always have an IF_DEFAULT=0
that falls back to IF_IDE.
I really dislike having the same list twice, just with different but almost
identical names.
Alex
Re: [Qemu-devel] [PATCH 06/24] Remove local ram_size that hides global one
On 19.11.2012, at 14:35, Eduardo Habkost wrote: > On Mon, Nov 19, 2012 at 02:14:27PM +0100, Alexander Graf wrote: >> >> On 13.11.2012, at 13:11, Christian Borntraeger wrote: >> >>> From: Heinz Graalfs >>> >>> The global variable 'ram_size' is hidden by the local variable >>> declaration in s390_init() >> >> That's the point of Eduardo's patch, no? Or do we need access to the global >> to change its value afterwards? If so, please write a reasonable patch >> description that actually explains the problem. >> > > Actually, I wanted to keep the existing behavior (whatever it was), and > use local variables to replace the old function parameters, and change > nothing else. Most machines used a local variable named 'ram_size', but > the parameter to s390_init() was named 'my_ram_size'. > > So, considering that this at least restores the previous behavior: > > Reviewed-by: Eduardo Habkost Yup, Heinz, please resend with a patch description that actually tells us what the problem is. Alex
Re: [Qemu-devel] [PATCH 1/1] Support default block interfaces per QEMUMachine
On 19/11/12 14:36, Alexander Graf wrote:
>
> On 12.11.2012, at 09:22, Christian Borntraeger wrote:
>
>> There are QEMUMachines that have neither IF_IDE nor IF_SCSI as a
>> default/standard interface to their block devices / drives. Therfore,
>> this patch introduces a new field default_block per QEMUMachine struct.
>> The prior use_scsi field becomes thereby obsolete and is replaced through
>> .default_block = DEFAULT_SCSI.
>>
>> Based on an initial patch from Einar Lueck
>>
>> Signed-off-by: Christian Borntraeger
>> ---
>> blockdev.c |4 ++--
>> blockdev.h | 29 -
>> hw/boards.h |3 ++-
>> hw/device-hotplug.c |2 +-
>> hw/highbank.c |2 +-
>> hw/leon3.c |1 -
>> hw/mips_jazz.c |4 ++--
>> hw/pc_sysfw.c |2 +-
>> hw/puv3.c |1 -
>> hw/realview.c |7 ---
>> hw/s390-virtio.c| 16 +---
>> hw/spapr.c |2 +-
>> hw/sun4m.c | 24
>> hw/versatilepb.c|4 ++--
>> hw/vexpress.c |4 ++--
>> hw/xilinx_zynq.c|2 +-
>> vl.c| 20 +++-
>> 17 files changed, 71 insertions(+), 56 deletions(-)
>>
>> diff --git a/blockdev.c b/blockdev.c
>> index e73fd6e..aca3c14 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -275,7 +275,7 @@ static bool do_check_io_limits(BlockIOLimit *io_limits)
>> return true;
>> }
>>
>> -DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
>> +DriveInfo *drive_init(QemuOpts *opts, BlockDefault block_default)
>> {
>> const char *buf;
>> const char *file = NULL;
>> @@ -325,7 +325,7 @@ DriveInfo *drive_init(QemuOpts *opts, int
>> default_to_scsi)
>> return NULL;
>> }
>> } else {
>> -type = default_to_scsi ? IF_SCSI : IF_IDE;
>> +type = block_if_from_default(block_default);
>> }
>>
>> max_devs = if_max_devs[type];
>> diff --git a/blockdev.h b/blockdev.h
>> index 5f27b64..aba6d77 100644
>> --- a/blockdev.h
>> +++ b/blockdev.h
>> @@ -24,6 +24,33 @@ typedef enum {
>> IF_COUNT
>> } BlockInterfaceType;
>>
>> +/* For machine default interface. */
>> +typedef enum {
>> +DEFAULT_IDE = 0,
>> +DEFAULT_SCSI,
>> +DEFAULT_FLOPPY,
>> +DEFAULT_PFLASH,
>> +DEFAULT_MTD,
>> +DEFAULT_SD,
>> +DEFAULT_VIRTIO,
>> +DEFAULT_XEN
>> +} BlockDefault;
>
> Why a new enum? Can't we just reuse the IF_ ones?
>
> Also, traditionally Markus has had very strong opinions on anything IF_ and
> -drive related. It's probably a good idea to get him in the loop :).
>
> Alex
Review feedback from the first cycle. Anthony wanted to make the default (field
is not specified at all) a
sane default, which means IDE must be 0.
Re: [Qemu-devel] no monitor after disable vnc
Il 19/11/2012 14:25, Peter Cheung ha scritto: > Dear All > When i compile qemu in ubuntu 12.10 with "./configure > --target-list=i386-softmmu --prefix=/root/qemu --enable-debug > --disable-vnc --disable-werror" , after i start qemu, no screen output, > how to enable it? Please attach your config.log. Paolo
Re: [Qemu-devel] [PATCH 1/1] Support default block interfaces per QEMUMachine
On 12.11.2012, at 09:22, Christian Borntraeger wrote:
> There are QEMUMachines that have neither IF_IDE nor IF_SCSI as a
> default/standard interface to their block devices / drives. Therfore,
> this patch introduces a new field default_block per QEMUMachine struct.
> The prior use_scsi field becomes thereby obsolete and is replaced through
> .default_block = DEFAULT_SCSI.
>
> Based on an initial patch from Einar Lueck
>
> Signed-off-by: Christian Borntraeger
> ---
> blockdev.c |4 ++--
> blockdev.h | 29 -
> hw/boards.h |3 ++-
> hw/device-hotplug.c |2 +-
> hw/highbank.c |2 +-
> hw/leon3.c |1 -
> hw/mips_jazz.c |4 ++--
> hw/pc_sysfw.c |2 +-
> hw/puv3.c |1 -
> hw/realview.c |7 ---
> hw/s390-virtio.c| 16 +---
> hw/spapr.c |2 +-
> hw/sun4m.c | 24
> hw/versatilepb.c|4 ++--
> hw/vexpress.c |4 ++--
> hw/xilinx_zynq.c|2 +-
> vl.c| 20 +++-
> 17 files changed, 71 insertions(+), 56 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index e73fd6e..aca3c14 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -275,7 +275,7 @@ static bool do_check_io_limits(BlockIOLimit *io_limits)
> return true;
> }
>
> -DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
> +DriveInfo *drive_init(QemuOpts *opts, BlockDefault block_default)
> {
> const char *buf;
> const char *file = NULL;
> @@ -325,7 +325,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
> return NULL;
> }
> } else {
> -type = default_to_scsi ? IF_SCSI : IF_IDE;
> +type = block_if_from_default(block_default);
> }
>
> max_devs = if_max_devs[type];
> diff --git a/blockdev.h b/blockdev.h
> index 5f27b64..aba6d77 100644
> --- a/blockdev.h
> +++ b/blockdev.h
> @@ -24,6 +24,33 @@ typedef enum {
> IF_COUNT
> } BlockInterfaceType;
>
> +/* For machine default interface. */
> +typedef enum {
> +DEFAULT_IDE = 0,
> +DEFAULT_SCSI,
> +DEFAULT_FLOPPY,
> +DEFAULT_PFLASH,
> +DEFAULT_MTD,
> +DEFAULT_SD,
> +DEFAULT_VIRTIO,
> +DEFAULT_XEN
> +} BlockDefault;
Why a new enum? Can't we just reuse the IF_ ones?
Also, traditionally Markus has had very strong opinions on anything IF_ and
-drive related. It's probably a good idea to get him in the loop :).
Alex
> +
> +static inline BlockInterfaceType block_if_from_default(BlockDefault d)
> +{
> +switch (d) {
> +case DEFAULT_IDE:return IF_IDE;
> +case DEFAULT_SCSI: return IF_SCSI;
> +case DEFAULT_FLOPPY: return IF_FLOPPY;
> +case DEFAULT_PFLASH: return IF_PFLASH;
> +case DEFAULT_MTD:return IF_MTD;
> +case DEFAULT_SD: return IF_SD;
> +case DEFAULT_VIRTIO: return IF_VIRTIO;
> +case DEFAULT_XEN:return IF_XEN;
> +default: return IF_COUNT; /* will cause errors */
> +}
> +}
> +
> struct DriveInfo {
> BlockDriverState *bdrv;
> char *id;
> @@ -51,7 +78,7 @@ DriveInfo *drive_get_by_blockdev(BlockDriverState *bs);
> QemuOpts *drive_def(const char *optstr);
> QemuOpts *drive_add(BlockInterfaceType type, int index, const char *file,
> const char *optstr);
> -DriveInfo *drive_init(QemuOpts *arg, int default_to_scsi);
> +DriveInfo *drive_init(QemuOpts *arg, BlockDefault block_default);
>
> /* device-hotplug */
>
> diff --git a/hw/boards.h b/hw/boards.h
> index 813d0e5..04017bf 100644
> --- a/hw/boards.h
> +++ b/hw/boards.h
> @@ -3,6 +3,7 @@
> #ifndef HW_BOARDS_H
> #define HW_BOARDS_H
>
> +#include "blockdev.h"
> #include "qdev.h"
>
> typedef struct QEMUMachineInitArgs {
> @@ -24,7 +25,7 @@ typedef struct QEMUMachine {
> const char *desc;
> QEMUMachineInitFunc *init;
> QEMUMachineResetFunc *reset;
> -int use_scsi;
> +BlockDefault block_default;
> int max_cpus;
> unsigned int no_serial:1,
> no_parallel:1,
> diff --git a/hw/device-hotplug.c b/hw/device-hotplug.c
> index eec0fe3..7f5be92 100644
> --- a/hw/device-hotplug.c
> +++ b/hw/device-hotplug.c
> @@ -39,7 +39,7 @@ DriveInfo *add_init_drive(const char *optstr)
> if (!opts)
> return NULL;
>
> -dinfo = drive_init(opts, current_machine->use_scsi);
> +dinfo = drive_init(opts, current_machine->block_default);
> if (!dinfo) {
> qemu_opts_del(opts);
> return NULL;
> diff --git a/hw/highbank.c b/hw/highbank.c
> index afbb005..de1027b 100644
> --- a/hw/highbank.c
> +++ b/hw/highbank.c
> @@ -326,7 +326,7 @@ static QEMUMachine highbank_machine = {
> .name = "highbank",
> .desc = "Calxeda Highbank (ECX-1000)",
> .init = highbank_init,
> -.use_scsi = 1,
> +.block_default = DEFAULT_SCSI,
> .max_cpus = 4,
> };
>
> diff --git a/hw/leon3.c b/hw/leon3.c
> index 7742738..ef83dff 100644
> --- a/hw/leon3.c
> +++ b/hw/leon3.c
> @@ -212,7 +21
Re: [Qemu-devel] [PATCH 06/24] Remove local ram_size that hides global one
On Mon, Nov 19, 2012 at 02:14:27PM +0100, Alexander Graf wrote:
>
> On 13.11.2012, at 13:11, Christian Borntraeger wrote:
>
> > From: Heinz Graalfs
> >
> > The global variable 'ram_size' is hidden by the local variable
> > declaration in s390_init()
>
> That's the point of Eduardo's patch, no? Or do we need access to the global
> to change its value afterwards? If so, please write a reasonable patch
> description that actually explains the problem.
>
Actually, I wanted to keep the existing behavior (whatever it was), and
use local variables to replace the old function parameters, and change
nothing else. Most machines used a local variable named 'ram_size', but
the parameter to s390_init() was named 'my_ram_size'.
So, considering that this at least restores the previous behavior:
Reviewed-by: Eduardo Habkost
>
> Alex
>
> >
> > Signed-off-by: Heinz Graalfs
> > Signed-off-by: Christian Borntraeger
> > ---
> > hw/s390-virtio.c |1 -
> > 1 file changed, 1 deletion(-)
> >
> > diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
> > index ebec844..78477af 100644
> > --- a/hw/s390-virtio.c
> > +++ b/hw/s390-virtio.c
> > @@ -155,7 +155,6 @@ unsigned s390_del_running_cpu(CPUS390XState *env)
> > static void s390_init(QEMUMachineInitArgs *args)
> > {
> > ram_addr_t my_ram_size = args->ram_size;
> > -ram_addr_t ram_size = args->ram_size;
> > const char *cpu_model = args->cpu_model;
> > const char *kernel_filename = args->kernel_filename;
> > const char *kernel_cmdline = args->kernel_cmdline;
> > --
> > 1.7.10.1
> >
>
--
Eduardo
Re: [Qemu-devel] [PATCH 2/3] s390: Virtual channel subsystem support.
On 31.10.2012, at 17:24, Cornelia Huck wrote:
> Provide a mechanism for qemu to provide fully virtual subchannels to
> the guest. In the KVM case, this relies on the kernel's css support
> for I/O and machine check interrupt handling. The !KVM case handles
> interrupts on its own.
>
> Signed-off-by: Cornelia Huck
> ---
> hw/s390x/Makefile.objs |1 +
> hw/s390x/css.c | 1209
> hw/s390x/css.h | 90
> target-s390x/Makefile.objs |2 +-
> target-s390x/cpu.h | 232 +
> target-s390x/helper.c | 146 ++
> target-s390x/ioinst.c | 737 +++
> target-s390x/ioinst.h | 213
> target-s390x/kvm.c | 251 -
> target-s390x/misc_helper.c |6 +-
> 10 files changed, 2872 insertions(+), 15 deletions(-)
> create mode 100644 hw/s390x/css.c
> create mode 100644 hw/s390x/css.h
> create mode 100644 target-s390x/ioinst.c
> create mode 100644 target-s390x/ioinst.h
>
> diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs
> index 096dfcd..378b099 100644
> --- a/hw/s390x/Makefile.objs
> +++ b/hw/s390x/Makefile.objs
> @@ -4,3 +4,4 @@ obj-y := $(addprefix ../,$(obj-y))
> obj-y += sclp.o
> obj-y += event-facility.o
> obj-y += sclpquiesce.o sclpconsole.o
> +obj-y += css.o
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> new file mode 100644
> index 000..9adffb3
> --- /dev/null
> +++ b/hw/s390x/css.c
> @@ -0,0 +1,1209 @@
> +/*
> + * Channel subsystem base support.
> + *
> + * Copyright 2012 IBM Corp.
> + * Author(s): Cornelia Huck
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or (at
> + * your option) any later version. See the COPYING file in the top-level
> + * directory.
> + */
> +
> +#include "qemu-thread.h"
> +#include "qemu-queue.h"
> +#include
> +#include "bitops.h"
> +#include "kvm.h"
> +#include "cpu.h"
> +#include "ioinst.h"
> +#include "css.h"
> +#include "virtio-ccw.h"
> +
> +typedef struct CrwContainer {
> +CRW crw;
> +QTAILQ_ENTRY(CrwContainer) sibling;
> +} CrwContainer;
> +
> +typedef struct ChpInfo {
> +uint8_t in_use;
> +uint8_t type;
> +uint8_t is_virtual;
> +} ChpInfo;
> +
> +typedef struct SubchSet {
> +SubchDev *sch[MAX_SCHID + 1];
> +unsigned long schids_used[BITS_TO_LONGS(MAX_SCHID + 1)];
> +unsigned long devnos_used[BITS_TO_LONGS(MAX_SCHID + 1)];
> +} SubchSet;
> +
> +typedef struct CssImage {
> +SubchSet *sch_set[MAX_SSID + 1];
> +ChpInfo chpids[MAX_CHPID + 1];
> +} CssImage;
> +
> +typedef struct ChannelSubSys {
> +QTAILQ_HEAD(, CrwContainer) pending_crws;
> +bool do_crw_mchk;
> +bool crws_lost;
> +uint8_t max_cssid;
> +uint8_t max_ssid;
> +bool chnmon_active;
> +uint64_t chnmon_area;
> +CssImage *css[MAX_CSSID + 1];
> +uint8_t default_cssid;
> +} ChannelSubSys;
> +
> +static ChannelSubSys *channel_subsys;
> +
> +int css_create_css_image(uint8_t cssid, bool default_image)
> +{
> +if (cssid > MAX_CSSID) {
> +return -EINVAL;
> +}
> +if (channel_subsys->css[cssid]) {
> +return -EBUSY;
> +}
> +channel_subsys->css[cssid] = g_try_malloc0(sizeof(CssImage));
> +if (!channel_subsys->css[cssid]) {
> +return -ENOMEM;
> +}
> +if (default_image) {
> +channel_subsys->default_cssid = cssid;
> +}
> +return 0;
> +}
> +
> +static void css_write_phys_pmcw(uint64_t addr, PMCW *pmcw)
> +{
> +int i;
> +uint32_t offset = 0;
> +struct copy_pmcw {
> +uint32_t intparm;
> +uint16_t flags;
> +uint16_t devno;
> +uint8_t lpm;
> +uint8_t pnom;
> +uint8_t lpum;
> +uint8_t pim;
> +uint16_t mbi;
> +uint8_t pom;
> +uint8_t pam;
> +uint8_t chpid[8];
> +uint32_t chars;
> +} *copy;
This needs to be packed. Also, it might be a good idea to separate the struct
definition from the actual code ;).
> +
> +copy = (struct copy_pmcw *)pmcw;
This will break on any system that doesn't coincidently stick to the same
bitfield order as s390x. Please drop any usage of bitfields in QEMU source code
:).
> +stl_phys(addr + offset, copy->intparm);
> +offset += sizeof(copy->intparm);
Can't you just use cpu_physical_memory_map() and assign things left and right
as you see fit? Or prepare the target endianness struct on the stack and
cpu_physical_memory_read/write it from/to guest memory.
Also, please split this patch into smaller patches :). As it is now it's very
hard to review. However, apart from the above issues (which may happen in other
places of the code further down, I just didn't comment then) I couldn't see
major problems so far. But please split it nevertheless so that I have an
easier time reviewing it :)
Alex
[Qemu-devel] no monitor after disable vnc
Dear All When i compile qemu in ubuntu 12.10 with "./configure --target-list=i386-softmmu --prefix=/root/qemu --enable-debug --disable-vnc --disable-werror" , after i start qemu, no screen output, how to enable it? Thanksfrom Peter
Re: [Qemu-devel] [PATCH 06/24] Remove local ram_size that hides global one
On 19 November 2012 13:14, Alexander Graf wrote: > > On 13.11.2012, at 13:11, Christian Borntraeger wrote: > >> From: Heinz Graalfs >> >> The global variable 'ram_size' is hidden by the local variable >> declaration in s390_init() > > That's the point of Eduardo's patch, no? Or do we need access > to the global to change its value afterwards? The function later does: /* lets propagate the changed ram size into the global variable. */ ram_size = my_ram_size; which frankly I think is pretty nasty but then we've never had a very good mechanism for specifying board-specific restrictions around the memory size. -- PMM
Re: [Qemu-devel] [PATCH 06/24] Remove local ram_size that hides global one
On 13.11.2012, at 13:11, Christian Borntraeger wrote:
> From: Heinz Graalfs
>
> The global variable 'ram_size' is hidden by the local variable
> declaration in s390_init()
That's the point of Eduardo's patch, no? Or do we need access to the global to
change its value afterwards? If so, please write a reasonable patch description
that actually explains the problem.
Alex
>
> Signed-off-by: Heinz Graalfs
> Signed-off-by: Christian Borntraeger
> ---
> hw/s390-virtio.c |1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
> index ebec844..78477af 100644
> --- a/hw/s390-virtio.c
> +++ b/hw/s390-virtio.c
> @@ -155,7 +155,6 @@ unsigned s390_del_running_cpu(CPUS390XState *env)
> static void s390_init(QEMUMachineInitArgs *args)
> {
> ram_addr_t my_ram_size = args->ram_size;
> -ram_addr_t ram_size = args->ram_size;
> const char *cpu_model = args->cpu_model;
> const char *kernel_filename = args->kernel_filename;
> const char *kernel_cmdline = args->kernel_cmdline;
> --
> 1.7.10.1
>
Re: [Qemu-devel] scsi-hd with discard_granularity and unmap results in Aborted Commands
Il 19/11/2012 14:01, Stefan Priebe - Profihost AG ha scritto: >> The right behavior is to return >> only after the target says whether the cancellation was done or not. >> For libiscsi, it was implemented by the commits you mention. > > So the whole bunch of changes is needed for rbd? Something like the first three: 1bd075f29ea6d11853475c7c42734595720c3ac6 cfb3f5064af2d2e29c976e292c9472dfe9d61e31 27cbd828c617944c0f9603763fdf4fa87e7ad923 Paolo
